Skip to content

fix SonarCube issues "Local variables should not shadow class fields"(squid:HiddenFieldCheck) #11705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

igor-suhorukov
Copy link
Contributor

@igor-suhorukov igor-suhorukov commented Jan 20, 2018

To get rid confusing to know whether the field or the local variable is being used. This pull request fix 171 issues with "local variables overriding class field names"( squid:HiddenFieldCheck)

This pull request is related issues described in ticket #11650

To get rid confusing to know whether the field or the local variable is being used.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 20, 2018
@wilkinsona
Copy link
Member

Thanks for the PR but I disagree with this one. We prefix all field access with this so, IMO, there’s no confusion.

@wilkinsona wilkinsona added status: on-hold We can't start working on this issue yet for: team-attention An issue we'd like other members of the team to review labels Jan 20, 2018
@igor-suhorukov
Copy link
Contributor Author

@wilkinsona ok, it is my mistake maybe

@igor-suhorukov
Copy link
Contributor Author

@wilkinsona
Copy link
Member

Certainly not a mistake, but a lot of Sonar’s rules are quite subjective so we may not always agree. If we proceed with #11650, we’ll want to spend time creating a profile that team are happy with and hopefully all agree on.

@philwebb
Copy link
Member

I'm with Andy on this one I'm afraid. We have a checkstyle rule to enforce the this. prefix so I think there's limited value in enforcing different local variable names. In fact, I often prefer the simpler names you get when you aren't forced to choose something different.

@philwebb philwebb closed this Jan 21, 2018
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: on-hold We can't start working on this issue yet status: waiting-for-triage An issue we've not yet triaged labels Jan 21, 2018
@philwebb
Copy link
Member

what do you think about Utility classes should not have public constructors

Generally agree and we have a checkstyle rule for this. The problem with the Sonar results here is that is' picking things like CacheMeterBinderProvidersConfiguration that I don't consider utility classes. In fact, we've generally tried to avoid util classes altogether in Boot.

Generic wildcard types should not be used in return parameters

This one is very subjective as well. I generally try not to return them for public APIs, but it can be difficult to always do this without causing a lot of problems. Take this one for example. We could return a PropertySource<Object> I guess, but really what's the point? We're really saying you can return a property source of any generic here and we'll deal with it. I'm not sure always enforcing the rule makes for better code, although it may well be worth a review of some APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants