----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55236/#review160734 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/pdx/internal/PdxReaderImpl.java (line 890) <https://reviews.apache.org/r/55236/#comment231931> The name of this method is a bit confusing. It is an OBJECT field. You are checking if it contains a String. A better name would be "getPdxStringFromObjectField". You should javadoc that it will return null if the given field does not contain a String. I think it would be even better if this method did the test "ft.getFieldType() == FieldType.STRING" instead of the caller doing it. Then the caller would just look like this: PdxString pdxString = getPdxStringFromObjectField(ft); if (pdxString != null) { return pdxString; } return readField(field); geode-core/src/main/java/org/apache/geode/pdx/internal/PdxReaderImpl.java (line 900) <https://reviews.apache.org/r/55236/#comment231933> Get rid of this first "if". All you need is the condition on the "else if". geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java (line 66) <https://reviews.apache.org/r/55236/#comment231934> I think this change is confusing. PdxInstanceHelper has an add method for every type of pdx field. Wouldn't it be better to have the one caller of this method to instead call addObjectField instead of changing the meaning of addStringField? - Darrel Schneider On Jan. 5, 2017, 3:14 p.m., Hitesh Khamesra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55236/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2017, 3:14 p.m.) > > > Review request for geode, Bruce Schuchardt and Udo Kohlmeyer. > > > Repository: geode > > > Description > ------- > > GEODE-2271 JSOnFormatter can generate three pdxTypeId for one json field. > > JSON field can have 3 values(fieldValue, NULL, fieldNotExist), this > causes 3 pdxTypeIds. To reuduce this we merged first two values in > one pdxType. > > Added unit test for it > > > Diffs > ----- > > geode-core/src/main/java/org/apache/geode/pdx/internal/PdxReaderImpl.java > 4ebc33d > > geode-core/src/main/java/org/apache/geode/pdx/internal/json/PdxInstanceHelper.java > e957cd6 > geode-core/src/test/java/org/apache/geode/pdx/Employee.java cfb46b5 > geode-core/src/test/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java > 979da13 > geode-core/src/test/java/org/apache/geode/pdx/PdxStringJUnitTest.java > c072e14 > > geode-core/src/test/java/org/apache/geode/pdx/TestObjectForJSONFormatter.java > ca0abc3 > > Diff: https://reviews.apache.org/r/55236/diff/ > > > Testing > ------- > > > Thanks, > > Hitesh Khamesra > >
