[GitHub] [commons-lang] kinow commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…

["kinow (via GitHub)" <gi...@apache.org>]

kinow commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r1160238001


##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -131,7 +167,15 @@ private boolean accept(final Field field) {
         if (Modifier.isTransient(field.getModifiers())) {
             return false;
         }
-        return !Modifier.isStatic(field.getModifiers());
+        if (Modifier.isStatic(field.getModifiers())) {
+            return false;
+        }
+        if (this.excludeFieldNames != null

Review Comment:
   Hmm, I think I didn't understand the whole discussion here.
   
   @garydgregory pointed that instead of having a default of `null`, we could have something like `private String[] excludeFieldNames = ArrayUtils.EMPTY_STRING_ARRAY;`
   
   >If you do invoke the method setExcludeFieldNames() it will have a default value of null, and in that cause a NPE here.
   
   That sounds incorrect, I think (it's late, so I could be wrong). If you call `setExcludeFieldNames`, there's a check if the value passed is `null`, and `this.excludeFieldNames = ArrayUtils.EMPTY_STRING_ARRAY;`. Thus, `excludeFieldNames` cannot be `null`.
   
   So if you did as @garydgregory suggested, and if we keep the defensive check for `null` in `setExcludeFieldNames`, then I believe what @garydgregory initially suggested holds here? 
   
   >If you make the default value the empty array and the setter does not allow excludeFieldNames to be null, then you do not need this check? It might be better this way though, not sure.
   
   Then, I think the question would be whether it's worth having the `binarySearch` knowing it's an empty array (maybe we could skip if if we know it's an empty array).



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org