WW-5279 Improve readability of XmlConfigurationProvider class #657
Conversation
|
|
||
| final XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o; | ||
|
|
||
| XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o; |
There was a problem hiding this comment.
| XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o; | |
| final XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o; |
this could remain final
There was a problem hiding this comment.
I find it clutters the code and goes against present common practice - but I can revert these changes if that is the preferred code style.
There was a problem hiding this comment.
https://stackoverflow.com/questions/154314/when-should-one-use-final-for-method-parameters-and-local-variables/154510#154510 LGTM, but for this existing line "If I'm in someone else's code, I'm not going to pull them out but if I'm writing new code I won't put them in." would fit.
With your explanation, both styles would be ok for me. Maybe @lukaszlenart has some recommendation according to the Struts coding style?
There was a problem hiding this comment.
I'm fine with dropping final in this context, I like using finals for fields and function arguments, where it makes sense :)
| LOG.error("Unable to verify action [{}] with class [{}], from [{}]", name, className, location); | ||
| return; | ||
| } | ||
| if (!className.isEmpty() && !verifyAction(className, name, location)) { |
There was a problem hiding this comment.
Maybe we can remove the unused param name from verifyAction L:490
Sonar says: Parameter 'name' is never used
There was a problem hiding this comment.
There's multiple methods in this class with unused params - I've kept them to maintain compatibility with potential subclasses. Perhaps something to revisit when releasing the next major version.
There was a problem hiding this comment.
@sepe81 I'm right now creating a task to mark a given code @Deprecated and then another task (targeting major/minor release) to remove the code. I found such approach more useful and more informative for the users :)
WW-5279