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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
