[GitHub] [struts] sepe81 commented on a diff in pull request #657: WW-5279 Improve readability of XmlConfigurationProvider class

[via GitHub]

sepe81 commented on code in PR #657:
URL: https://github.com/apache/struts/pull/657#discussion_r1091847537


##
core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java:
##
@@ -164,13 +174,10 @@ public boolean equals(Object o) {
 if (this == o) {
 return true;
 }
-
 if (!(o instanceof XmlConfigurationProvider)) {
 return false;
 }
-
-final XmlConfigurationProvider xmlConfigurationProvider = 
(XmlConfigurationProvider) o;
-
+XmlConfigurationProvider xmlConfigurationProvider = 
(XmlConfigurationProvider) o;

Review 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?



-- 
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: issues-unsubscr...@struts.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [struts] sepe81 commented on a diff in pull request #657: WW-5279 Improve readability of XmlConfigurationProvider class

[via GitHub]

sepe81 commented on code in PR #657:
URL: https://github.com/apache/struts/pull/657#discussion_r1090851725


##
core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java:
##
@@ -439,28 +445,16 @@ protected void addAction(Element actionElement, 
PackageConfig.Builder packageCon
 String name = actionElement.getAttribute("name");
 String className = actionElement.getAttribute("class");
 //methodName should be null if it's not set
-String methodName = 
StringUtils.trimToNull(actionElement.getAttribute("method"));
+String methodName = trimToNull(actionElement.getAttribute("method"));
 Location location = DomHelper.getLocationObject(actionElement);
 
 if (location == null) {
 LOG.warn("Location null for {}", className);
 }
 
-// if there isn't a class name specified for an  then try to
-// use the default-class-ref from the 
-if (StringUtils.isEmpty(className)) {
-// if there is a package default-class-ref use that, otherwise use 
action support
-   /* if (StringUtils.isNotEmpty(packageContext.getDefaultClassRef())) 
{
-className = packageContext.getDefaultClassRef();
-} else {
-className = ActionSupport.class.getName();
-}*/
-
-} else {
-if (!verifyAction(className, name, location)) {
-LOG.error("Unable to verify action [{}] with class [{}], from 
[{}]", name, className, location);
-return;
-}
+if (!className.isEmpty() && !verifyAction(className, name, location)) {

Review Comment:
   Maybe we can remove the unused param `name` from verifyAction L:490
   
   Sonar says: Parameter 'name' is never used



##
core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java:
##
@@ -164,13 +174,10 @@ public boolean equals(Object o) {
 if (this == o) {
 return true;
 }
-
 if (!(o instanceof XmlConfigurationProvider)) {
 return false;
 }
-
-final XmlConfigurationProvider xmlConfigurationProvider = 
(XmlConfigurationProvider) o;
-
+XmlConfigurationProvider xmlConfigurationProvider = 
(XmlConfigurationProvider) o;

Review Comment:
   ```suggestion
   final XmlConfigurationProvider xmlConfigurationProvider = 
(XmlConfigurationProvider) o;
   ```
   
   this could remain final



-- 
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: issues-unsubscr...@struts.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org