> 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?
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. - 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 > >
