Issue #13809: Kill mutation in TokenUtil#13943
Conversation
| .filter(fld -> Modifier.isPublic(fld.getModifiers()) && fld.getType() == Integer.TYPE) | ||
| .collect(Collectors.toUnmodifiableMap( | ||
| Field::getName, fld -> getIntFromField(fld, fld.getName())) | ||
| Field::getName, fld -> getIntFromField(fld, fld)) |
There was a problem hiding this comment.
getIntFromField is basically a wrapper for Field#getInt. From reading the docs of Field, it looks like we should be passing the class itself to this method. I am not sure how this ever worked in the first place. Please do:
| Field::getName, fld -> getIntFromField(fld, fld)) | |
| Field::getName, fld -> getIntFromField(fld, cls)) |
There was a problem hiding this comment.
I am not sure how this ever worked in the first place
This is because we are going against a static field. It has no instance to speak of, so most likely reflection is ignoring the 2nd parameter we pass. We also have similar funky uses in TokenUtilTest as getIntFromField is public when it should not be.
Truthfully this 2nd parameter should be null, as there is no instance.
| Field::getName, fld -> getIntFromField(fld, fld)) | |
| Field::getName, fld -> getIntFromField(fld, null)) |
Also nothing else but 1 method uses getIntFromField and we are now saying the 2nd parameter is completely unnecessary. getIntFromField needs to be refactored.
835eaa5 to
5da8930
Compare
|
@Kevin222004 , can we put null in second parameter to make it clear it is to be ignored. |
5da8930 to
aae0d32
Compare
romani
left a comment
There was a problem hiding this comment.
Ok to merge.
@Kevin222004 , please update Checker config
aae0d32 to
1c38d03
Compare
nrmancuso
left a comment
There was a problem hiding this comment.
I really don't see any other method in java.lang.reflect.Field that we can use to avoid second parameter. I am good with null to make it clear that the second parameter doesn't matter in the case of static fields.
|
Assigned to @rnveach to complete review, I am not sure if we want to open a ticket for refactor of |
|
@Kevin222004 If you want an easy assignment, |
Issue #13809: Kill mutation in TokenUtil
Explaintaion
checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/utils/TokenUtil.java
Line 80 in 0322ba1
we are passing object as a param so I think it will not make a difference either we pass only name or whole field