sanjanarampurkottur01 commented on PR #218: URL: https://github.com/apache/commons-dbutils/pull/218#issuecomment-1826313580
Hello Gary, I am working on these code changes, based on your review. Thank you. Best Regards, Sanjana Rampur Kottur On Fri, 24 Nov 2023 at 09:04, Gary Gregory ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > @sanjanarampurkottur01 <https://github.com/sanjanarampurkottur01> > Please see my comments. > ------------------------------ > > In > src/main/java/org/apache/commons/dbutils/wrappers/StringTrimmedResultSet.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404328661> > : > > > @@ -92,12 +92,25 @@ public Object invoke(final Object proxy, final Method method, final Object[] arg > Object result = method.invoke(this.resultSet, args); > > if (result instanceof String > - && (method.getName().equals("getObject") > - || method.getName().equals("getString"))) { > + && isMethodNameGetObjectOrString(method.getName())) { > > No need for this complication and code bloat when there is only one case > for reuse. > ------------------------------ > > In > src/main/java/org/apache/commons/dbutils/wrappers/StringTrimmedResultSet.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404329087> > : > > > result = ((String) result).trim(); > } > > return result; > } > > + private boolean isMethodNameGetObjectOrString(String name) { > + return isMethodNameGetObject(name) || isMethodNameGetString(name); > + } > + > + private boolean isMethodNameGetObject(String name) { > + final String getObjectName = "getObject"; > > Useless local variable (see above, code bloat). > ------------------------------ > > In > src/main/java/org/apache/commons/dbutils/wrappers/StringTrimmedResultSet.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404329152> > : > > > result = ((String) result).trim(); > } > > return result; > } > > + private boolean isMethodNameGetObjectOrString(String name) { > + return isMethodNameGetObject(name) || isMethodNameGetString(name); > + } > + > + private boolean isMethodNameGetObject(String name) { > + final String getObjectName = "getObject"; > + return name.equals(getObjectName); > + } > + > + private boolean isMethodNameGetString(String name) { > + final String getStringName = "getString"; > > Useless local variable (see above, code bloat). > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404329347> > : > > > @@ -186,21 +187,21 @@ public void setTestField(final int idx, final Integer testField) { > } > } > > - private static final BeanProcessor beanProc = new BeanProcessor(); > + private static final BeanProcessor BEAN_PROCESSOR = new BeanProcessor(); > > Good catch! :-) > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404329605> > : > > > > public void testCheckAnnotationOnMissingReadMethod() throws Exception { > final String[] colNames = {"testField"}; > final ResultSetMetaData metaData = MockResultSetMetaData.create(colNames); > > - final String testField = "first"; > + final String testFieldHeaderData = "first"; > > No need for this wordiness. > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404330141> > : > > > }; > > - final ResultSet rs = MockResultSet.create(metaData, rows); > - assertTrue(rs.next()); > + final ResultSet resultSet = MockResultSet.create(metaData, rows); > > rs -> resultSet: Good one :-) > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404330685> > : > > > > - final String name = "first"; > - final List<String> things = Arrays.asList("1", "2", "3", "4"); > - final List<String> stuff = things; > + final String nameHeaderData = "first"; > > No need for this wordiness. > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404331570> > : > > > }; > > - final ResultSet rs = MockResultSet.create(metaData, rows); > - assertTrue(rs.next()); > - IndexedPropertyTestClass testCls = new IndexedPropertyTestClass(); > - testCls = beanProc.populateBean(rs, testCls); > - assertEquals(name, testCls.getName()); > - assertArrayEquals(things.toArray(), testCls.getThings().toArray()); > - assertArrayEquals(stuff.toArray(), testCls.getStuff().toArray()); > + final ResultSet resultSet = MockResultSet.create(metaData, rows); > > rs -> resultSet: Good one :-) > Don't use "Data" in names, everything is "data". > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404333124> > : > > > for (int i = 1; i < columns.length; i++) { > - assertTrue(columns[i] != BeanProcessor.PROPERTY_NOT_FOUND); > + assertNotEquals(columns[i], BeanProcessor.PROPERTY_NOT_FOUND); > > You're NOT testing the same thing! The old test uses object identity! > Review all your changes to make sure you do not change assertion semantics! > Or, are you saying this one assertion was incorrect? > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404333521> > : > > > } > > public void testMapColumnToAnnotationField() throws Exception { > final String[] columnNames = { "test", "test", "three_" }; > final String[] columnLabels = { "one", "two", null }; > - final ResultSetMetaData rsmd = ProxyFactory.instance().createResultSetMetaData( > - new MockResultSetMetaData(columnNames, columnLabels)); > + final ResultSetMetaData resultSetMetaData = buildResultSetMetaData(columnNames, columnLabels); > > resultSetMetaData: Good one :-) > ------------------------------ > > In src/test/java/org/apache/commons/dbutils/BeanProcessorTest.java > <https://github.com/apache/commons-dbutils/pull/218#discussion_r1404333893> > : > > > @@ -308,15 +306,21 @@ public void testWrongSetterParamCount() throws Exception { > final String[] colNames = {"testField"}; > final ResultSetMetaData metaData = MockResultSetMetaData.create(colNames); > > - final Integer testField = 1; > + final Integer testFieldHeaderData = 1; > > testField is fine. > > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/commons-dbutils/pull/218#pullrequestreview-1747911892>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/BDTL25YZQ44WBZ4722NCLBDYGCLPDAVCNFSM6AAAAAA7RHKLBWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBXHEYTCOBZGI> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
