WW-5279 Improve readability of XmlConfigurationProvider class by kusalk · Pull Request #657 · apache/struts (original) (raw)

sepe81

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not easy to overlook the big diff, but good to see the cognitive complexity reduced afterwards
final XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o;
XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o;
final XmlConfigurationProvider xmlConfigurationProvider = (XmlConfigurationProvider) o;

this could remain final

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can remove the unused param name from verifyAction L:490

Sonar says: Parameter 'name' is never used

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)