geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Udo Kohlmeyer <ukohlme...@gmail.com>
Subject Re: Review Request 55736: GEODE-2328: Refactor add Field methods to reduce the duplicates
Date Wed, 01 Feb 2017 21:03:30 GMT


> On Jan. 31, 2017, 11:43 p.m., Bruce Schuchardt wrote:
> > The only thing that concerns me about this change-set is the auto-boxing/unboxing
that's going on with the new method.  The old code didn't do that.
> 
> Hitesh Khamesra wrote:
>     This also concerns me.
> 
> Galen O'Sullivan wrote:
>     Don't `writeData` and `filldata` do exactly the same auto-boxing/unboxing? It's a
little hard to follow because there's both removing of duplicate methods and condensing of
multiple methods into one (might have been easier to read as two pull requests), but I think
it comes down to the same eventual code in the end. Unless you're seeing something I'm not?
> 
> Udo Kohlmeyer wrote:
>     I'm not sure that I'm seeing the autoboxing part that is being referred to. What
I do see is that the new code is type casting. Which in its own right could be problem, as
we'd only know about the type cast exception within the switch statement.
>     The whole intention with the refactor was to ensure that the "sorting" helper would
follow the same logic/flow/code as the unsorted helper. This way the code would be 100% the
same except for the temp collection to the sort the fields.

nevermind.. I see all the autoboxing.... The consolidated code could be written to be more
specific and less autoboxing...


- Udo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55736/#review163741
-----------------------------------------------------------


On Jan. 20, 2017, 4:50 p.m., Udo Kohlmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55736/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2017, 4:50 p.m.)
> 
> 
> Review request for geode, Bruce Schuchardt, Galen O'Sullivan, and Hitesh Khamesra.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> the add fields have the same behavior and identical code. Refactored the code to use
a single method and reduce duplication
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java
39d16a57e 
>   geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceSortedHelper.java
7f510da15 
>   geode-core/src/test/java/org/apache/geode/pdx/JSONPdxClientServerDUnitTest.java 396eb1d13

> 
> Diff: https://reviews.apache.org/r/55736/diff/
> 
> 
> Testing
> -------
> 
> precheckin - passed
> 
> 
> Thanks,
> 
> Udo Kohlmeyer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message