From bd4095ed41dbafb816a43fc8d1eb77ae0b244295 Mon Sep 17 00:00:00 2001 From: Arda Aslan Date: Mon, 12 Sep 2016 18:07:16 +0200 Subject: [PATCH 01/12] Added a naming rule called IsDoesNotReturnBoolean. According to article "Linguistics Antipatterns: What They Are and How Developers Perceive Them", a method starting with "isX" and not returning boolean introduces a code smell, reduces maintainability mainly. Add two linguistics codes smells. one related with methods and the other with variables (these two can be merged into 1 rule later) Feedbacks are appreciated. --- ...ttributeTypeAndNameIsInconsistentRule.java | 105 ++++++++ .../MethodTypeAndNameIsInconsistentRule.java | 107 +++++++++ .../resources/category/java/codestyle.xml | 225 ++++++++++++++++++ .../rule/codestyle/CodeStyleRulesTest.java | 2 + .../AttributeTypeAndNameIsInconsistent.xml | 70 ++++++ .../xml/MethodTypeAndNameIsInconsistent.xml | 181 ++++++++++++++ 6 files changed, 690 insertions(+) create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/AttributeTypeAndNameIsInconsistent.xml create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java new file mode 100644 index 0000000000..b22f6490a1 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java @@ -0,0 +1,105 @@ +package net.sourceforge.pmd.lang.java.rule.codestyle; + +import net.sourceforge.pmd.lang.java.ast.*; +import net.sourceforge.pmd.lang.java.rule.*; + +public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule +{ + public Object visit(ASTFieldDeclaration node, Object data) + { + String nameOfField = node.getVariableName(); + ASTType t = (ASTType) node.jjtGetChild(0); // Is first child always ASTType + + /**************** Type Should Be Boolean As Name Suggests *********************/ + + if(nameOfField.startsWith("is") && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a field called isotherm or so + + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && + (nameOfField.charAt(3) > 64) && (nameOfField.charAt(3) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && + (nameOfField.charAt(4) > 64) && (nameOfField.charAt(4) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if(nameOfField.startsWith("should") && + (nameOfField.charAt(6) > 64) && (nameOfField.charAt(6) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + /***************************************************************************/ + + return super.visit(node,data); + } + + + public Object visit(ASTLocalVariableDeclaration node, Object data) + { + String nameOfField = node.getVariableName(); + ASTType t = (ASTType) node.jjtGetChild(0); // Is first child always ASTType + + /**************** Type Should Be Boolean As Name Suggests *********************/ + + if(nameOfField.startsWith("is") && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a local variable called isotherm or so + + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && + (nameOfField.charAt(3) > 64) && (nameOfField.charAt(3) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && + (nameOfField.charAt(4) > 64) && (nameOfField.charAt(4) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if(nameOfField.startsWith("should") && + (nameOfField.charAt(6) > 64) && (nameOfField.charAt(6) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + /***************************************************************************/ + + return super.visit(node,data); + } + +} diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java new file mode 100644 index 0000000000..6fb453a723 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java @@ -0,0 +1,107 @@ + +package net.sourceforge.pmd.lang.java.rule.codestyle; + +import net.sourceforge.pmd.lang.java.ast.*; +import net.sourceforge.pmd.lang.java.rule.*; + +public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule +{ + public Object visit(ASTMethodDeclaration node, Object data) + { + String nameOfMethod = node.getMethodName(); + ASTType t = null; + ASTResultType rt = null; + if(node.getResultType().jjtGetNumChildren() != 0) //for non-void methods + { + t = (ASTType) node.getResultType().jjtGetChild(0); // Is first child always ASTType + rt = node.getResultType(); + } + else + { + rt = node.getResultType(); // for void methods + } + + /**************** Should Return Boolean As Name Suggests *********************/ + + if(nameOfMethod.startsWith("is") && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a method called isotherm or so + + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if((nameOfMethod.startsWith("has") || nameOfMethod.startsWith("can")) && + (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if((nameOfMethod.startsWith("have") || nameOfMethod.startsWith("will")) && + (nameOfMethod.charAt(4) > 64) && (nameOfMethod.charAt(4) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + else if(nameOfMethod.startsWith("should") && + (nameOfMethod.charAt(6) > 64) && (nameOfMethod.charAt(6) < 91)) + { + if(!(t.getType().getName().equals("boolean"))) + { + addViolation(data, node); + } + } + + + /***************************************************************************/ + + /**************** Should Return Void As Name Suggests *********************/ + + if(nameOfMethod.startsWith("set") && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after set a capital letter expected to not addViolation to a method called settings or so + { + if(!(rt.isVoid())) //set method shouldnt return any type except void linguistically + { + addViolation(data, node); + } + } + + /***************************************************************************/ + + /******* Should Return A Type As Name Suggests But It Returns void *********/ + + if(nameOfMethod.startsWith("get") && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after get a capital letter expected to not addViolation to a method called getaways or so + { + if(rt.isVoid()) //get method shouldnt return void linguistically + { + addViolation(data, node); + } + } + + if(nameOfMethod.contains("To")) // To in the middle somewhere + { + if(rt.isVoid()) //a transform method shouldnt return void linguistically + { + addViolation(data, node); + } + } + + else if(nameOfMethod.startsWith("to") && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after to a capital letter expected to not addViolation to a method called tokenize or so + { + if(rt.isVoid()) //a transform method shouldnt return void linguistically + { + addViolation(data, node); + } + } + + /***************************************************************************/ + + return super.visit(node,data); + } +} diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 01aae5f509..0ecb734311 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -45,6 +45,77 @@ public abstract class Foo { // should be AbstractFoo + + + Linguistics Antipattern - Attribute name and type is inconsistent linguistically + + 3 + + + + + + + + Linguistics Antipattern - Method name and return type is inconsistent linguistically + + 3 + + + + + + + + + First and third variable in every group shouldnt raise error but second should + 12 + + + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml new file mode 100644 index 0000000000..3c2e4364d7 --- /dev/null +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml @@ -0,0 +1,181 @@ + + + + + First and third method in every group shouldnt raise error but second should Groups are separated by comments + 10 + + + From 58446a2ecd2b687fb1213c18921878db41faa84b Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Jul 2018 14:02:25 +0200 Subject: [PATCH 02/12] Fix runtime errors (NPE, ClassCastException, IndexOutOfBounds) --- ...ttributeTypeAndNameIsInconsistentRule.java | 86 ++++++++++--------- .../MethodTypeAndNameIsInconsistentRule.java | 69 ++++++++------- 2 files changed, 81 insertions(+), 74 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java index b22f6490a1..8b2eb1a5f3 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java @@ -1,105 +1,109 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; -import net.sourceforge.pmd.lang.java.ast.*; -import net.sourceforge.pmd.lang.java.rule.*; +import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTType; +import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule -{ - public Object visit(ASTFieldDeclaration node, Object data) - { +public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule +{ + @Override + public Object visit(ASTFieldDeclaration node, Object data) + { String nameOfField = node.getVariableName(); - ASTType t = (ASTType) node.jjtGetChild(0); // Is first child always ASTType + ASTType t = node.getFirstChildOfType(ASTType.class); /**************** Type Should Be Boolean As Name Suggests *********************/ - - if(nameOfField.startsWith("is") && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a field called isotherm or so + + if(nameOfField.startsWith("is") && nameOfField.length() > 2 && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a field called isotherm or so { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && + + else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 && (nameOfField.charAt(3) > 64) && (nameOfField.charAt(3) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && + + else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 && (nameOfField.charAt(4) > 64) && (nameOfField.charAt(4) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if(nameOfField.startsWith("should") && + + else if(nameOfField.startsWith("should") && nameOfField.length() > 6 && (nameOfField.charAt(6) > 64) && (nameOfField.charAt(6) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - + /***************************************************************************/ - + return super.visit(node,data); } - - - public Object visit(ASTLocalVariableDeclaration node, Object data) - { + + + @Override + public Object visit(ASTLocalVariableDeclaration node, Object data) + { String nameOfField = node.getVariableName(); - ASTType t = (ASTType) node.jjtGetChild(0); // Is first child always ASTType + ASTType t = node.getFirstChildOfType(ASTType.class); /**************** Type Should Be Boolean As Name Suggests *********************/ - - if(nameOfField.startsWith("is") && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a local variable called isotherm or so + + if(nameOfField.startsWith("is") && nameOfField.length() > 2 && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a local variable called isotherm or so { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && + + else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 && (nameOfField.charAt(3) > 64) && (nameOfField.charAt(3) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && + + else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 && (nameOfField.charAt(4) > 64) && (nameOfField.charAt(4) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if(nameOfField.startsWith("should") && + + else if(nameOfField.startsWith("should") && nameOfField.length() > 6 && (nameOfField.charAt(6) > 64) && (nameOfField.charAt(6) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null & !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - + /***************************************************************************/ - + return super.visit(node,data); } - + } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java index 6fb453a723..6eae653968 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java @@ -1,89 +1,92 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; -import net.sourceforge.pmd.lang.java.ast.*; -import net.sourceforge.pmd.lang.java.rule.*; +import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTResultType; +import net.sourceforge.pmd.lang.java.ast.ASTType; +import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule +public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule { - public Object visit(ASTMethodDeclaration node, Object data) - { + @Override + public Object visit(ASTMethodDeclaration node, Object data) + { String nameOfMethod = node.getMethodName(); ASTType t = null; ASTResultType rt = null; if(node.getResultType().jjtGetNumChildren() != 0) //for non-void methods { - t = (ASTType) node.getResultType().jjtGetChild(0); // Is first child always ASTType + t = node.getResultType().getFirstChildOfType(ASTType.class); rt = node.getResultType(); } else { - rt = node.getResultType(); // for void methods + rt = node.getResultType(); // for void methods } - + /**************** Should Return Boolean As Name Suggests *********************/ - - if(nameOfMethod.startsWith("is") && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a method called isotherm or so + + if(nameOfMethod.startsWith("is") && nameOfMethod.length() > 2 && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a method called isotherm or so { - if(!(t.getType().getName().equals("boolean"))) + if(t != null && !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if((nameOfMethod.startsWith("has") || nameOfMethod.startsWith("can")) && + + else if((nameOfMethod.startsWith("has") || nameOfMethod.startsWith("can")) && nameOfMethod.length() > 3 && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null && !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if((nameOfMethod.startsWith("have") || nameOfMethod.startsWith("will")) && + + else if((nameOfMethod.startsWith("have") || nameOfMethod.startsWith("will")) && nameOfMethod.length() > 4 && (nameOfMethod.charAt(4) > 64) && (nameOfMethod.charAt(4) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null && !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - else if(nameOfMethod.startsWith("should") && + + else if(nameOfMethod.startsWith("should") && nameOfMethod.length() > 6 && (nameOfMethod.charAt(6) > 64) && (nameOfMethod.charAt(6) < 91)) { - if(!(t.getType().getName().equals("boolean"))) + if(t != null && !(t.getType().getName().equals("boolean"))) { addViolation(data, node); } } - - + + /***************************************************************************/ - + /**************** Should Return Void As Name Suggests *********************/ - - if(nameOfMethod.startsWith("set") && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after set a capital letter expected to not addViolation to a method called settings or so + + if(nameOfMethod.startsWith("set") && nameOfMethod.length() > 3 && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after set a capital letter expected to not addViolation to a method called settings or so { - if(!(rt.isVoid())) //set method shouldnt return any type except void linguistically + if(t != null && !(rt.isVoid())) //set method shouldnt return any type except void linguistically { addViolation(data, node); } } - + /***************************************************************************/ /******* Should Return A Type As Name Suggests But It Returns void *********/ - - if(nameOfMethod.startsWith("get") && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after get a capital letter expected to not addViolation to a method called getaways or so + + if(nameOfMethod.startsWith("get") && nameOfMethod.length() > 3 && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after get a capital letter expected to not addViolation to a method called getaways or so { if(rt.isVoid()) //get method shouldnt return void linguistically { addViolation(data, node); } } - + if(nameOfMethod.contains("To")) // To in the middle somewhere { if(rt.isVoid()) //a transform method shouldnt return void linguistically @@ -91,8 +94,8 @@ public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule addViolation(data, node); } } - - else if(nameOfMethod.startsWith("to") && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after to a capital letter expected to not addViolation to a method called tokenize or so + + else if(nameOfMethod.startsWith("to") && nameOfMethod.length() > 2 && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after to a capital letter expected to not addViolation to a method called tokenize or so { if(rt.isVoid()) //a transform method shouldnt return void linguistically { @@ -101,7 +104,7 @@ public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule } /***************************************************************************/ - + return super.visit(node,data); } } From 7a25d9a91084d65b5fd46ab1d92d066b2d09de33 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Jul 2018 14:26:04 +0200 Subject: [PATCH 03/12] Checkstyle, Pmd --- ...ttributeTypeAndNameIsInconsistentRule.java | 129 +++++++----------- .../MethodTypeAndNameIsInconsistentRule.java | 128 ++++++++--------- 2 files changed, 110 insertions(+), 147 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java index 8b2eb1a5f3..4149719651 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java @@ -1,3 +1,7 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + package net.sourceforge.pmd.lang.java.rule.codestyle; import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; @@ -5,105 +9,78 @@ import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule -{ +public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule { @Override - public Object visit(ASTFieldDeclaration node, Object data) - { + public Object visit(ASTFieldDeclaration node, Object data) { String nameOfField = node.getVariableName(); ASTType t = node.getFirstChildOfType(ASTType.class); /**************** Type Should Be Boolean As Name Suggests *********************/ - if(nameOfField.startsWith("is") && nameOfField.length() > 2 && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a field called isotherm or so - - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 && - (nameOfField.charAt(3) > 64) && (nameOfField.charAt(3) < 91)) - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 && - (nameOfField.charAt(4) > 64) && (nameOfField.charAt(4) < 91)) - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if(nameOfField.startsWith("should") && nameOfField.length() > 6 && - (nameOfField.charAt(6) > 64) && (nameOfField.charAt(6) < 91)) - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } + if (nameOfField.startsWith("is") && nameOfField.length() > 2 && nameOfField.charAt(2) > 64 + && nameOfField.charAt(2) < 91) { + // after is a capital letter expected to not addViolation to a field + // called isotherm or so + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if ((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 + && nameOfField.charAt(3) > 64 && nameOfField.charAt(3) < 91) { + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if ((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 + && nameOfField.charAt(4) > 64 && nameOfField.charAt(4) < 91) { + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if (nameOfField.startsWith("should") && nameOfField.length() > 6 && nameOfField.charAt(6) > 64 + && nameOfField.charAt(6) < 91) { + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } } /***************************************************************************/ - return super.visit(node,data); + return super.visit(node, data); } - @Override - public Object visit(ASTLocalVariableDeclaration node, Object data) - { + public Object visit(ASTLocalVariableDeclaration node, Object data) { String nameOfField = node.getVariableName(); ASTType t = node.getFirstChildOfType(ASTType.class); /**************** Type Should Be Boolean As Name Suggests *********************/ - if(nameOfField.startsWith("is") && nameOfField.length() > 2 && (nameOfField.charAt(2) > 64) && (nameOfField.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a local variable called isotherm or so + if (nameOfField.startsWith("is") && nameOfField.length() > 2 && nameOfField.charAt(2) > 64 + && nameOfField.charAt(2) < 91) { + // after is a capital letter expected to not addViolation to a local + // variable called isotherm or so - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 && - (nameOfField.charAt(3) > 64) && (nameOfField.charAt(3) < 91)) - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 && - (nameOfField.charAt(4) > 64) && (nameOfField.charAt(4) < 91)) - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if(nameOfField.startsWith("should") && nameOfField.length() > 6 && - (nameOfField.charAt(6) > 64) && (nameOfField.charAt(6) < 91)) - { - if(t != null & !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if ((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 + && nameOfField.charAt(3) > 64 && nameOfField.charAt(3) < 91) { + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if ((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 + && nameOfField.charAt(4) > 64 && nameOfField.charAt(4) < 91) { + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if (nameOfField.startsWith("should") && nameOfField.length() > 6 && nameOfField.charAt(6) > 64 + && nameOfField.charAt(6) < 91) { + if (t != null & !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } } /***************************************************************************/ - return super.visit(node,data); + return super.visit(node, data); } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java index 6eae653968..cba236ff43 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java @@ -1,3 +1,6 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ package net.sourceforge.pmd.lang.java.rule.codestyle; @@ -6,105 +9,88 @@ import net.sourceforge.pmd.lang.java.ast.ASTResultType; import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule -{ +public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule { @Override - public Object visit(ASTMethodDeclaration node, Object data) - { + public Object visit(ASTMethodDeclaration node, Object data) { String nameOfMethod = node.getMethodName(); ASTType t = null; ASTResultType rt = null; - if(node.getResultType().jjtGetNumChildren() != 0) //for non-void methods - { + if (node.getResultType().jjtGetNumChildren() != 0) { // for non-void methods t = node.getResultType().getFirstChildOfType(ASTType.class); rt = node.getResultType(); - } - else - { - rt = node.getResultType(); // for void methods + } else { + rt = node.getResultType(); // for void methods } /**************** Should Return Boolean As Name Suggests *********************/ - if(nameOfMethod.startsWith("is") && nameOfMethod.length() > 2 && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after is a capital letter expected to not addViolation to a method called isotherm or so + if (nameOfMethod.startsWith("is") && nameOfMethod.length() > 2 && nameOfMethod.charAt(2) > 64 + && nameOfMethod.charAt(2) < 91) { + // after is a capital letter expected to not addViolation to a method + // called isotherm or so - { - if(t != null && !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } + if (t != null && !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if ((nameOfMethod.startsWith("has") || nameOfMethod.startsWith("can")) && nameOfMethod.length() > 3 + && nameOfMethod.charAt(3) > 64 && nameOfMethod.charAt(3) < 91) { + if (t != null && !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if ((nameOfMethod.startsWith("have") || nameOfMethod.startsWith("will")) && nameOfMethod.length() > 4 + && nameOfMethod.charAt(4) > 64 && nameOfMethod.charAt(4) < 91) { + if (t != null && !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } + } else if (nameOfMethod.startsWith("should") && nameOfMethod.length() > 6 && nameOfMethod.charAt(6) > 64 + && nameOfMethod.charAt(6) < 91) { + if (t != null && !(t.getType().getName().equals("boolean"))) { + addViolation(data, node); + } } - else if((nameOfMethod.startsWith("has") || nameOfMethod.startsWith("can")) && nameOfMethod.length() > 3 && - (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) - { - if(t != null && !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if((nameOfMethod.startsWith("have") || nameOfMethod.startsWith("will")) && nameOfMethod.length() > 4 && - (nameOfMethod.charAt(4) > 64) && (nameOfMethod.charAt(4) < 91)) - { - if(t != null && !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - else if(nameOfMethod.startsWith("should") && nameOfMethod.length() > 6 && - (nameOfMethod.charAt(6) > 64) && (nameOfMethod.charAt(6) < 91)) - { - if(t != null && !(t.getType().getName().equals("boolean"))) - { - addViolation(data, node); - } - } - - /***************************************************************************/ /**************** Should Return Void As Name Suggests *********************/ - if(nameOfMethod.startsWith("set") && nameOfMethod.length() > 3 && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after set a capital letter expected to not addViolation to a method called settings or so - { - if(t != null && !(rt.isVoid())) //set method shouldnt return any type except void linguistically - { - addViolation(data, node); - } + if (nameOfMethod.startsWith("set") && nameOfMethod.length() > 3 && nameOfMethod.charAt(3) > 64 + && nameOfMethod.charAt(3) < 91) { + // after set a capital letter expected to not addViolation to a method + // called settings or so + + if (t != null && !(rt.isVoid())) { // set method shouldnt return any type except void linguistically + addViolation(data, node); + } } /***************************************************************************/ /******* Should Return A Type As Name Suggests But It Returns void *********/ - if(nameOfMethod.startsWith("get") && nameOfMethod.length() > 3 && (nameOfMethod.charAt(3) > 64) && (nameOfMethod.charAt(3) < 91)) // after get a capital letter expected to not addViolation to a method called getaways or so - { - if(rt.isVoid()) //get method shouldnt return void linguistically - { - addViolation(data, node); - } + if (nameOfMethod.startsWith("get") && nameOfMethod.length() > 3 && nameOfMethod.charAt(3) > 64 + && nameOfMethod.charAt(3) < 91) { + // after get a capital letter expected to not addViolation to a method + // called getaways or so + if (rt.isVoid()) { // get method shouldnt return void linguistically + addViolation(data, node); + } } - if(nameOfMethod.contains("To")) // To in the middle somewhere - { - if(rt.isVoid()) //a transform method shouldnt return void linguistically - { - addViolation(data, node); - } - } - - else if(nameOfMethod.startsWith("to") && nameOfMethod.length() > 2 && (nameOfMethod.charAt(2) > 64) && (nameOfMethod.charAt(2) < 91)) // after to a capital letter expected to not addViolation to a method called tokenize or so - { - if(rt.isVoid()) //a transform method shouldnt return void linguistically - { - addViolation(data, node); - } + if (nameOfMethod.contains("To")) { // To in the middle somewhere + if (rt.isVoid()) { // a transform method shouldnt return void linguistically + addViolation(data, node); + } + } else if (nameOfMethod.startsWith("to") && nameOfMethod.length() > 2 && nameOfMethod.charAt(2) > 64 + && nameOfMethod.charAt(2) < 91) { + // after to a capital letter expected to not addViolation to a method + // called tokenize or so + if (rt.isVoid()) { // a transform method shouldnt return void linguistically + addViolation(data, node); + } } /***************************************************************************/ - return super.visit(node,data); + return super.visit(node, data); } } From beeeb74efbe140d35681685c0662ecf46de657df Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Jul 2018 18:50:10 +0200 Subject: [PATCH 04/12] Simplify implementation of AttributeTypeAndNameIsInconsistent and MethodTypeAndNameIsInconsistent Split test cases --- ...ttributeTypeAndNameIsInconsistentRule.java | 105 ++--- .../MethodTypeAndNameIsInconsistentRule.java | 138 ++++--- .../AttributeTypeAndNameIsInconsistent.xml | 166 +++++--- .../xml/MethodTypeAndNameIsInconsistent.xml | 368 ++++++++++-------- 4 files changed, 415 insertions(+), 362 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java index 4149719651..f371d69433 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java @@ -4,83 +4,60 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule { + private static final Set PREFIXES; + + static { + final Set prefixCollection = new HashSet(); + prefixCollection.add("is"); + prefixCollection.add("has"); + prefixCollection.add("can"); + prefixCollection.add("have"); + prefixCollection.add("will"); + prefixCollection.add("should"); + PREFIXES = Collections.unmodifiableSet(prefixCollection); + } + + public AttributeTypeAndNameIsInconsistentRule() { + addRuleChainVisit(ASTFieldDeclaration.class); + addRuleChainVisit(ASTLocalVariableDeclaration.class); + } + + private void checkField(String nameOfField, Node node, Object data) { + ASTType type = node.getFirstChildOfType(ASTType.class); + if (type != null) { + for (String prefix : PREFIXES) { + if (nameOfField.startsWith(prefix) && nameOfField.length() > prefix.length() + && Character.isUpperCase(nameOfField.charAt(prefix.length()))) { + if (!"boolean".equals(type.getType().getName())) { + addViolation(data, node); + } + } + } + } + } + @Override public Object visit(ASTFieldDeclaration node, Object data) { String nameOfField = node.getVariableName(); - ASTType t = node.getFirstChildOfType(ASTType.class); - - /**************** Type Should Be Boolean As Name Suggests *********************/ - - if (nameOfField.startsWith("is") && nameOfField.length() > 2 && nameOfField.charAt(2) > 64 - && nameOfField.charAt(2) < 91) { - // after is a capital letter expected to not addViolation to a field - // called isotherm or so - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if ((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 - && nameOfField.charAt(3) > 64 && nameOfField.charAt(3) < 91) { - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if ((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 - && nameOfField.charAt(4) > 64 && nameOfField.charAt(4) < 91) { - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if (nameOfField.startsWith("should") && nameOfField.length() > 6 && nameOfField.charAt(6) > 64 - && nameOfField.charAt(6) < 91) { - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } - - /***************************************************************************/ - - return super.visit(node, data); + checkField(nameOfField, node, data); + return data; } @Override public Object visit(ASTLocalVariableDeclaration node, Object data) { String nameOfField = node.getVariableName(); - ASTType t = node.getFirstChildOfType(ASTType.class); - - /**************** Type Should Be Boolean As Name Suggests *********************/ - - if (nameOfField.startsWith("is") && nameOfField.length() > 2 && nameOfField.charAt(2) > 64 - && nameOfField.charAt(2) < 91) { - // after is a capital letter expected to not addViolation to a local - // variable called isotherm or so - - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if ((nameOfField.startsWith("has") || nameOfField.startsWith("can")) && nameOfField.length() > 3 - && nameOfField.charAt(3) > 64 && nameOfField.charAt(3) < 91) { - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if ((nameOfField.startsWith("have") || nameOfField.startsWith("will")) && nameOfField.length() > 4 - && nameOfField.charAt(4) > 64 && nameOfField.charAt(4) < 91) { - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if (nameOfField.startsWith("should") && nameOfField.length() > 6 && nameOfField.charAt(6) > 64 - && nameOfField.charAt(6) < 91) { - if (t != null & !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } - - /***************************************************************************/ - - return super.visit(node, data); + checkField(nameOfField, node, data); + return data; } - } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java index cba236ff43..cfda75ea0a 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java @@ -4,93 +4,87 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTResultType; import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule { + private static final Set BOOLEAN_PREFIXES; + + static { + final Set prefixCollection = new HashSet(); + prefixCollection.add("is"); + prefixCollection.add("has"); + prefixCollection.add("can"); + prefixCollection.add("have"); + prefixCollection.add("will"); + prefixCollection.add("should"); + BOOLEAN_PREFIXES = Collections.unmodifiableSet(prefixCollection); + } + + public MethodTypeAndNameIsInconsistentRule() { + addRuleChainVisit(ASTMethodDeclaration.class); + } + @Override public Object visit(ASTMethodDeclaration node, Object data) { String nameOfMethod = node.getMethodName(); - ASTType t = null; - ASTResultType rt = null; - if (node.getResultType().jjtGetNumChildren() != 0) { // for non-void methods - t = node.getResultType().getFirstChildOfType(ASTType.class); - rt = node.getResultType(); - } else { - rt = node.getResultType(); // for void methods + + checkBooleanMethods(node, data, nameOfMethod); + checkSetters(node, data, nameOfMethod); + checkGetters(node, data, nameOfMethod); + checkTransformMethods(node, data, nameOfMethod); + + return data; + } + + private void checkTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (nameOfMethod.contains("To") && resultType.isVoid()) { + // To in the middle somewhere + // a transform method shouldn't return void linguistically + addViolation(data, node); + } else if (hasPrefix(nameOfMethod, "to") && resultType.isVoid()) { + // a transform method shouldn't return void linguistically + addViolation(data, node); } + } - /**************** Should Return Boolean As Name Suggests *********************/ + private void checkGetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (hasPrefix(nameOfMethod, "get") && resultType.isVoid()) { + // get method shouldn't return void linguistically + addViolation(data, node); + } + } - if (nameOfMethod.startsWith("is") && nameOfMethod.length() > 2 && nameOfMethod.charAt(2) > 64 - && nameOfMethod.charAt(2) < 91) { - // after is a capital letter expected to not addViolation to a method - // called isotherm or so + private void checkSetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (hasPrefix(nameOfMethod, "set") && !resultType.isVoid()) { + // set method shouldn't return any type except void linguistically + addViolation(data, node); + } + } - if (t != null && !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if ((nameOfMethod.startsWith("has") || nameOfMethod.startsWith("can")) && nameOfMethod.length() > 3 - && nameOfMethod.charAt(3) > 64 && nameOfMethod.charAt(3) < 91) { - if (t != null && !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if ((nameOfMethod.startsWith("have") || nameOfMethod.startsWith("will")) && nameOfMethod.length() > 4 - && nameOfMethod.charAt(4) > 64 && nameOfMethod.charAt(4) < 91) { - if (t != null && !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); - } - } else if (nameOfMethod.startsWith("should") && nameOfMethod.length() > 6 && nameOfMethod.charAt(6) > 64 - && nameOfMethod.charAt(6) < 91) { - if (t != null && !(t.getType().getName().equals("boolean"))) { - addViolation(data, node); + private void checkBooleanMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + ASTType t = node.getResultType().getFirstChildOfType(ASTType.class); + if (!resultType.isVoid() && t != null) { + for (String prefix : BOOLEAN_PREFIXES) { + if (hasPrefix(nameOfMethod, prefix) && !"boolean".equals(t.getType().getName())) { + addViolation(data, node); + } } } + } - /***************************************************************************/ - - /**************** Should Return Void As Name Suggests *********************/ - - if (nameOfMethod.startsWith("set") && nameOfMethod.length() > 3 && nameOfMethod.charAt(3) > 64 - && nameOfMethod.charAt(3) < 91) { - // after set a capital letter expected to not addViolation to a method - // called settings or so - - if (t != null && !(rt.isVoid())) { // set method shouldnt return any type except void linguistically - addViolation(data, node); - } - } - - /***************************************************************************/ - - /******* Should Return A Type As Name Suggests But It Returns void *********/ - - if (nameOfMethod.startsWith("get") && nameOfMethod.length() > 3 && nameOfMethod.charAt(3) > 64 - && nameOfMethod.charAt(3) < 91) { - // after get a capital letter expected to not addViolation to a method - // called getaways or so - if (rt.isVoid()) { // get method shouldnt return void linguistically - addViolation(data, node); - } - } - - if (nameOfMethod.contains("To")) { // To in the middle somewhere - if (rt.isVoid()) { // a transform method shouldnt return void linguistically - addViolation(data, node); - } - } else if (nameOfMethod.startsWith("to") && nameOfMethod.length() > 2 && nameOfMethod.charAt(2) > 64 - && nameOfMethod.charAt(2) < 91) { - // after to a capital letter expected to not addViolation to a method - // called tokenize or so - if (rt.isVoid()) { // a transform method shouldnt return void linguistically - addViolation(data, node); - } - } - - /***************************************************************************/ - - return super.visit(node, data); + private static boolean hasPrefix(String name, String prefix) { + return name.startsWith(prefix) && name.length() > prefix.length() + && Character.isUpperCase(name.charAt(prefix.length())); } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/AttributeTypeAndNameIsInconsistent.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/AttributeTypeAndNameIsInconsistent.xml index be53efcb80..9bf68fad27 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/AttributeTypeAndNameIsInconsistent.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/AttributeTypeAndNameIsInconsistent.xml @@ -5,66 +5,116 @@ xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> - First and third variable in every group shouldnt raise error but second should - 12 + Prefix is + 2 + 3,8 + ]]> + + + + Prefix has + 2 + 3,8 + + + + + Prefix can + 2 + 3,8 + + + + + Prefix will + 2 + 3,8 + + + + + Prefix have + 2 + 3,8 + + + + + Prefix should + 2 + 3,8 + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml index 3c2e4364d7..37326cd5a7 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml @@ -4,178 +4,210 @@ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd"> + - First and third method in every group shouldnt raise error but second should Groups are separated by comments - 10 + Prefix is + 1 + 6 + ]]> + + + + public class MethodTypeAndNameIsInconsistentWithPrefixHas + 1 + 6 + + + + + Prefix Have + 1 + 6 + + + + + Prefix can + 1 + 6 + + + + + Prefix will + 1 + 6 + + + + + Prefix should + 1 + 6 + + + + + Setters + 1 + 6 + + + + + Getters + 1 + 6 + + + + + Prefix to and contains To + 1 + 6 + + + + + Contains To + 1 + 2 + From ede63f0bcb950458c415a45a27185b23e50c9311 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Jul 2018 19:18:34 +0200 Subject: [PATCH 05/12] Introduce property booleanPrefixes --- ...ttributeTypeAndNameIsInconsistentRule.java | 25 ++++++------------- .../MethodTypeAndNameIsInconsistentRule.java | 25 ++++++------------- 2 files changed, 16 insertions(+), 34 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java index f371d69433..928956e335 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java @@ -4,31 +4,22 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; - import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; +import net.sourceforge.pmd.properties.StringMultiProperty; public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule { - private static final Set PREFIXES; - - static { - final Set prefixCollection = new HashSet(); - prefixCollection.add("is"); - prefixCollection.add("has"); - prefixCollection.add("can"); - prefixCollection.add("have"); - prefixCollection.add("will"); - prefixCollection.add("should"); - PREFIXES = Collections.unmodifiableSet(prefixCollection); - } + private static final StringMultiProperty BOOLEAN_PREFIXES_PROPERTY = StringMultiProperty.named("booleanPrefixes") + .defaultValues("is", "has", "can", "have", "will", "should") + .desc("the prefixes of fields that return boolean") + .uiOrder(1.0f) + .build(); public AttributeTypeAndNameIsInconsistentRule() { + definePropertyDescriptor(BOOLEAN_PREFIXES_PROPERTY); addRuleChainVisit(ASTFieldDeclaration.class); addRuleChainVisit(ASTLocalVariableDeclaration.class); } @@ -36,7 +27,7 @@ public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule { private void checkField(String nameOfField, Node node, Object data) { ASTType type = node.getFirstChildOfType(ASTType.class); if (type != null) { - for (String prefix : PREFIXES) { + for (String prefix : getProperty(BOOLEAN_PREFIXES_PROPERTY)) { if (nameOfField.startsWith(prefix) && nameOfField.length() > prefix.length() && Character.isUpperCase(nameOfField.charAt(prefix.length()))) { if (!"boolean".equals(type.getType().getName())) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java index cfda75ea0a..89412104ca 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java @@ -4,30 +4,21 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; - import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTResultType; import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; +import net.sourceforge.pmd.properties.StringMultiProperty; public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule { - private static final Set BOOLEAN_PREFIXES; - - static { - final Set prefixCollection = new HashSet(); - prefixCollection.add("is"); - prefixCollection.add("has"); - prefixCollection.add("can"); - prefixCollection.add("have"); - prefixCollection.add("will"); - prefixCollection.add("should"); - BOOLEAN_PREFIXES = Collections.unmodifiableSet(prefixCollection); - } + private static final StringMultiProperty BOOLEAN_PREFIXES_PROPERTY = StringMultiProperty.named("booleanPrefixes") + .defaultValues("is", "has", "can", "have", "will", "should") + .desc("the prefixes of methods that return boolean") + .uiOrder(1.0f) + .build(); public MethodTypeAndNameIsInconsistentRule() { + definePropertyDescriptor(BOOLEAN_PREFIXES_PROPERTY); addRuleChainVisit(ASTMethodDeclaration.class); } @@ -75,7 +66,7 @@ public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule { ASTResultType resultType = node.getResultType(); ASTType t = node.getResultType().getFirstChildOfType(ASTType.class); if (!resultType.isVoid() && t != null) { - for (String prefix : BOOLEAN_PREFIXES) { + for (String prefix : getProperty(BOOLEAN_PREFIXES_PROPERTY)) { if (hasPrefix(nameOfMethod, prefix) && !"boolean".equals(t.getType().getName())) { addViolation(data, node); } From a02bfe88611072431f0a1fcb4f2c4728fa37bb92 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Jul 2018 19:46:25 +0200 Subject: [PATCH 06/12] Combine the two rules into LinguisticNamingRule --- .../main/resources/rulesets/releases/660.xml | 1 + ...ttributeTypeAndNameIsInconsistentRule.java | 54 ---- .../rule/codestyle/LinguisticNamingRule.java | 148 +++++++++ .../MethodTypeAndNameIsInconsistentRule.java | 81 ----- .../resources/category/java/codestyle.xml | 284 ++++-------------- .../rule/codestyle/CodeStyleRulesTest.java | 3 +- .../AttributeTypeAndNameIsInconsistent.xml | 120 -------- ...sInconsistent.xml => LinguisticNaming.xml} | 134 ++++++++- 8 files changed, 333 insertions(+), 492 deletions(-) delete mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java delete mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java delete mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/AttributeTypeAndNameIsInconsistent.xml rename pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/{MethodTypeAndNameIsInconsistent.xml => LinguisticNaming.xml} (54%) diff --git a/pmd-core/src/main/resources/rulesets/releases/660.xml b/pmd-core/src/main/resources/rulesets/releases/660.xml index bb7d39faf2..de3f2cceec 100644 --- a/pmd-core/src/main/resources/rulesets/releases/660.xml +++ b/pmd-core/src/main/resources/rulesets/releases/660.xml @@ -9,6 +9,7 @@ This ruleset contains links to rules that are new in PMD v6.6.0 + diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java deleted file mode 100644 index 928956e335..0000000000 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/AttributeTypeAndNameIsInconsistentRule.java +++ /dev/null @@ -1,54 +0,0 @@ -/** - * BSD-style license; for more info see http://pmd.sourceforge.net/license.html - */ - -package net.sourceforge.pmd.lang.java.rule.codestyle; - -import net.sourceforge.pmd.lang.ast.Node; -import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTType; -import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.properties.StringMultiProperty; - -public class AttributeTypeAndNameIsInconsistentRule extends AbstractJavaRule { - private static final StringMultiProperty BOOLEAN_PREFIXES_PROPERTY = StringMultiProperty.named("booleanPrefixes") - .defaultValues("is", "has", "can", "have", "will", "should") - .desc("the prefixes of fields that return boolean") - .uiOrder(1.0f) - .build(); - - public AttributeTypeAndNameIsInconsistentRule() { - definePropertyDescriptor(BOOLEAN_PREFIXES_PROPERTY); - addRuleChainVisit(ASTFieldDeclaration.class); - addRuleChainVisit(ASTLocalVariableDeclaration.class); - } - - private void checkField(String nameOfField, Node node, Object data) { - ASTType type = node.getFirstChildOfType(ASTType.class); - if (type != null) { - for (String prefix : getProperty(BOOLEAN_PREFIXES_PROPERTY)) { - if (nameOfField.startsWith(prefix) && nameOfField.length() > prefix.length() - && Character.isUpperCase(nameOfField.charAt(prefix.length()))) { - if (!"boolean".equals(type.getType().getName())) { - addViolation(data, node); - } - } - } - } - } - - @Override - public Object visit(ASTFieldDeclaration node, Object data) { - String nameOfField = node.getVariableName(); - checkField(nameOfField, node, data); - return data; - } - - @Override - public Object visit(ASTLocalVariableDeclaration node, Object data) { - String nameOfField = node.getVariableName(); - checkField(nameOfField, node, data); - return data; - } -} diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java new file mode 100644 index 0000000000..cb4e3ef693 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java @@ -0,0 +1,148 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.codestyle; + +import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTResultType; +import net.sourceforge.pmd.lang.java.ast.ASTType; +import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; +import net.sourceforge.pmd.properties.BooleanProperty; +import net.sourceforge.pmd.properties.StringMultiProperty; + +public class LinguisticNamingRule extends AbstractJavaRule { + private static final BooleanProperty CHECK_BOOLEAN_METHODS = BooleanProperty.named("checkBooleanMethod") + .defaultValue(true).desc("Check method names and types for inconsistent naming").uiOrder(1.0f).build(); + private static final BooleanProperty CHECK_GETTERS = BooleanProperty.named("checkGetters").defaultValue(true) + .desc("Check return type of getters").uiOrder(2.0f).build(); + private static final BooleanProperty CHECK_SETTERS = BooleanProperty.named("checkSetters").defaultValue(true) + .desc("Check return type of setters").uiOrder(3.0f).build(); + private static final BooleanProperty CHECK_TRANSFORM_METHODS = BooleanProperty.named("checkTransformMethods") + .defaultValue(true).desc("Check return type of transform methods").uiOrder(4.0f).build(); + private static final StringMultiProperty BOOLEAN_METHOD_PREFIXES_PROPERTY = StringMultiProperty + .named("booleanMethodPrefixes").defaultValues("is", "has", "can", "have", "will", "should") + .desc("the prefixes of methods that return boolean").uiOrder(5.0f).build(); + + private static final BooleanProperty CHECK_FIELDS = BooleanProperty.named("checkFields").defaultValue(true) + .desc("Check field names and types for inconsistent naming").uiOrder(6.0f).build(); + private static final BooleanProperty CHECK_VARIABLES = BooleanProperty.named("checkVariables").defaultValue(true) + .desc("Check local variable names and types for inconsistent naming").uiOrder(7.0f).build(); + private static final StringMultiProperty BOOLEAN_FIELD_PREFIXES_PROPERTY = StringMultiProperty + .named("booleanFieldPrefixes").defaultValues("is", "has", "can", "have", "will", "should") + .desc("the prefixes of fields that return boolean").uiOrder(8.0f).build(); + + public LinguisticNamingRule() { + definePropertyDescriptor(CHECK_BOOLEAN_METHODS); + definePropertyDescriptor(CHECK_GETTERS); + definePropertyDescriptor(CHECK_SETTERS); + definePropertyDescriptor(CHECK_TRANSFORM_METHODS); + definePropertyDescriptor(BOOLEAN_METHOD_PREFIXES_PROPERTY); + definePropertyDescriptor(CHECK_FIELDS); + definePropertyDescriptor(CHECK_VARIABLES); + definePropertyDescriptor(BOOLEAN_FIELD_PREFIXES_PROPERTY); + addRuleChainVisit(ASTMethodDeclaration.class); + addRuleChainVisit(ASTFieldDeclaration.class); + addRuleChainVisit(ASTLocalVariableDeclaration.class); + } + + @Override + public Object visit(ASTMethodDeclaration node, Object data) { + String nameOfMethod = node.getMethodName(); + + if (getProperty(CHECK_BOOLEAN_METHODS)) { + checkBooleanMethods(node, data, nameOfMethod); + } + + if (getProperty(CHECK_SETTERS)) { + checkSetters(node, data, nameOfMethod); + } + + if (getProperty(CHECK_GETTERS)) { + checkGetters(node, data, nameOfMethod); + } + + if (getProperty(CHECK_TRANSFORM_METHODS)) { + checkTransformMethods(node, data, nameOfMethod); + } + + return data; + } + + private void checkTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (nameOfMethod.contains("To") && resultType.isVoid()) { + // To in the middle somewhere + // a transform method shouldn't return void linguistically + addViolation(data, node); + } else if (hasPrefix(nameOfMethod, "to") && resultType.isVoid()) { + // a transform method shouldn't return void linguistically + addViolation(data, node); + } + } + + private void checkGetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (hasPrefix(nameOfMethod, "get") && resultType.isVoid()) { + // get method shouldn't return void linguistically + addViolation(data, node); + } + } + + private void checkSetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (hasPrefix(nameOfMethod, "set") && !resultType.isVoid()) { + // set method shouldn't return any type except void linguistically + addViolation(data, node); + } + } + + private void checkBooleanMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + ASTType t = node.getResultType().getFirstChildOfType(ASTType.class); + if (!resultType.isVoid() && t != null) { + for (String prefix : getProperty(BOOLEAN_METHOD_PREFIXES_PROPERTY)) { + if (hasPrefix(nameOfMethod, prefix) && !"boolean".equals(t.getType().getName())) { + addViolation(data, node); + } + } + } + } + + private void checkField(String nameOfField, Node node, Object data) { + ASTType type = node.getFirstChildOfType(ASTType.class); + if (type != null) { + for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { + if (hasPrefix(nameOfField, prefix) && !"boolean".equals(type.getType().getName())) { + addViolation(data, node); + } + } + } + } + + @Override + public Object visit(ASTFieldDeclaration node, Object data) { + String nameOfField = node.getVariableName(); + if (getProperty(CHECK_FIELDS)) { + checkField(nameOfField, node, data); + } + return data; + } + + @Override + public Object visit(ASTLocalVariableDeclaration node, Object data) { + String nameOfField = node.getVariableName(); + if (getProperty(CHECK_VARIABLES)) { + checkField(nameOfField, node, data); + } + return data; + } + + private static boolean hasPrefix(String name, String prefix) { + return name.startsWith(prefix) && name.length() > prefix.length() + && Character.isUpperCase(name.charAt(prefix.length())); + } +} diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java deleted file mode 100644 index 89412104ca..0000000000 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/MethodTypeAndNameIsInconsistentRule.java +++ /dev/null @@ -1,81 +0,0 @@ -/** - * BSD-style license; for more info see http://pmd.sourceforge.net/license.html - */ - -package net.sourceforge.pmd.lang.java.rule.codestyle; - -import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTResultType; -import net.sourceforge.pmd.lang.java.ast.ASTType; -import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.properties.StringMultiProperty; - -public class MethodTypeAndNameIsInconsistentRule extends AbstractJavaRule { - private static final StringMultiProperty BOOLEAN_PREFIXES_PROPERTY = StringMultiProperty.named("booleanPrefixes") - .defaultValues("is", "has", "can", "have", "will", "should") - .desc("the prefixes of methods that return boolean") - .uiOrder(1.0f) - .build(); - - public MethodTypeAndNameIsInconsistentRule() { - definePropertyDescriptor(BOOLEAN_PREFIXES_PROPERTY); - addRuleChainVisit(ASTMethodDeclaration.class); - } - - @Override - public Object visit(ASTMethodDeclaration node, Object data) { - String nameOfMethod = node.getMethodName(); - - checkBooleanMethods(node, data, nameOfMethod); - checkSetters(node, data, nameOfMethod); - checkGetters(node, data, nameOfMethod); - checkTransformMethods(node, data, nameOfMethod); - - return data; - } - - private void checkTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { - ASTResultType resultType = node.getResultType(); - if (nameOfMethod.contains("To") && resultType.isVoid()) { - // To in the middle somewhere - // a transform method shouldn't return void linguistically - addViolation(data, node); - } else if (hasPrefix(nameOfMethod, "to") && resultType.isVoid()) { - // a transform method shouldn't return void linguistically - addViolation(data, node); - } - } - - private void checkGetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { - ASTResultType resultType = node.getResultType(); - if (hasPrefix(nameOfMethod, "get") && resultType.isVoid()) { - // get method shouldn't return void linguistically - addViolation(data, node); - } - } - - private void checkSetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { - ASTResultType resultType = node.getResultType(); - if (hasPrefix(nameOfMethod, "set") && !resultType.isVoid()) { - // set method shouldn't return any type except void linguistically - addViolation(data, node); - } - } - - private void checkBooleanMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { - ASTResultType resultType = node.getResultType(); - ASTType t = node.getResultType().getFirstChildOfType(ASTType.class); - if (!resultType.isVoid() && t != null) { - for (String prefix : getProperty(BOOLEAN_PREFIXES_PROPERTY)) { - if (hasPrefix(nameOfMethod, prefix) && !"boolean".equals(t.getType().getName())) { - addViolation(data, node); - } - } - } - } - - private static boolean hasPrefix(String name, String prefix) { - return name.startsWith(prefix) && name.length() > prefix.length() - && Character.isUpperCase(name.charAt(prefix.length())); - } -} diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 0ecb734311..0b5b44dd29 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -45,77 +45,6 @@ public abstract class Foo { // should be AbstractFoo - - - Linguistics Antipattern - Attribute name and type is inconsistent linguistically - - 3 - - - - - + + + This rule finds Linguistic Naming Antipatterns. It checks for fields, that are named, as if they should + be boolean but have a different type. It also checks for methods, that according to their name, should + return a boolean, but don't. Further, it checks, that getters and transform methods return something, + but setters won't. + + For more information, see [Linguistic Antipatterns - What They Are and How +Developers Perceive Them](https://doi.org/10.1007/s10664-014-9350-8). + + 3 + + + + + - - - Linguistics Antipattern - Method name and return type is inconsistent linguistically - - 3 - - - - - - - - - Prefix is - 2 - 3,8 - - - - - Prefix has - 2 - 3,8 - - - - - Prefix can - 2 - 3,8 - - - - - Prefix will - 2 - 3,8 - - - - - Prefix have - 2 - 3,8 - - - - - Prefix should - 2 - 3,8 - - - diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml similarity index 54% rename from pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml rename to pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml index 37326cd5a7..326e6d3bcc 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/MethodTypeAndNameIsInconsistent.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml @@ -6,7 +6,7 @@ - Prefix is + Method Prefix is 1 6 - public class MethodTypeAndNameIsInconsistentWithPrefixHas + Method Prefix Has 1 6 - Prefix Have + Method Prefix Have 1 6 - Prefix can + Method Prefix can 1 6 - Prefix will + Method Prefix will 1 6 - Prefix should + Method Prefix should 1 6 - Setters + Method Setters 1 6 - Getters + Method Getters 1 6 - Prefix to and contains To + Method Prefix to 1 6 - Contains To + Method Contains To 1 2 + + + + Field/Variable Prefix is + 2 + 3,8 + + + + + Field/Variable Prefix has + 2 + 3,8 + + + + + Field/Variable Prefix can + 2 + 3,8 + + + + + Field/Variable Prefix will + 2 + 3,8 + + + + + Field/Variable Prefix have + 2 + 3,8 + + + + + Field/Variable Prefix should + 2 + 3,8 + From 0ee6b54d38a2873779f1a863ce1189c27bfdb722 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 21 Jul 2018 19:51:18 +0200 Subject: [PATCH 07/12] Update release notes, mention new rule, refs #109, the original PR --- docs/pages/release_notes.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 23bf24e76d..68028e1bad 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -22,6 +22,11 @@ This is a minor release. #### New Rules +* The new Java rule [`LinguisticNaming`](pmd_rules_java_codestyle.html#linguisticnaming) (`java-codestyle`) + detects cases, when a method name indicates it returns a boolean (such as `isSmall()`) but it doesn't. + Besides method names, the rule also checks field and variable names. It also checks, that getters return + something but setters won't. The rule has several properties with which it can be customized. + * The new Java rule [`LocalVariableNamingConventions`](pmd_rules_java_codestyle.html#localvariablenamingconventions) (`java-codestlye`) detects local variable names that don't comply to a given convention. It defaults to standrd Java convention of using camelCase, but can be configured. Special cases can be configured for final variables and catched exceptions' names. @@ -54,5 +59,6 @@ This is a minor release. ### External Contributions +* [#109](https://github.com/pmd/pmd/pull/109): \[java] Add two linguistics rules under naming - [Arda Aslan](https://github.com/ardaasln) * [#1182](https://github.com/pmd/pmd/pull/1182): \[ui] XPath AutoComplete - [Akshat Bahety](https://github.com/akshatbahety) * [#1231](https://github.com/pmd/pmd/pull/1231): \[doc] Minor typo fix in installation.md - [Ashish Rana](https://github.com/ashishrana160796) From 62254aa6c2ffeb4f326b61de1f8056167714832b Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 6 Aug 2018 20:54:16 +0200 Subject: [PATCH 08/12] Fix review comments for LinguisticNamingRule * Added specific messages for the different types of linguistic patterns detected * Fix support for multiple field/variable declaration --- .../lang/java/ast/ASTVariableDeclarator.java | 5 ++ .../rule/codestyle/LinguisticNamingRule.java | 77 +++++++++++------- .../resources/category/java/codestyle.xml | 14 ++-- .../rule/codestyle/xml/LinguisticNaming.xml | 79 ++++++++++++++++++- 4 files changed, 137 insertions(+), 38 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java index 60fdd00c24..21d9b3d8ea 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java @@ -21,4 +21,9 @@ public class ASTVariableDeclarator extends AbstractJavaTypeNode { public Object jjtAccept(JavaParserVisitor visitor, Object data) { return visitor.visit(this, data); } + + public String getName() { + // first child will be VariableDeclaratorId + return jjtGetChild(0).getImage(); + } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java index cb4e3ef693..10e4e309f9 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java @@ -4,12 +4,14 @@ package net.sourceforge.pmd.lang.java.rule.codestyle; -import net.sourceforge.pmd.lang.ast.Node; +import java.util.List; + import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTResultType; import net.sourceforge.pmd.lang.java.ast.ASTType; +import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.properties.BooleanProperty; import net.sourceforge.pmd.properties.StringMultiProperty; @@ -33,7 +35,7 @@ public class LinguisticNamingRule extends AbstractJavaRule { .desc("Check local variable names and types for inconsistent naming").uiOrder(7.0f).build(); private static final StringMultiProperty BOOLEAN_FIELD_PREFIXES_PROPERTY = StringMultiProperty .named("booleanFieldPrefixes").defaultValues("is", "has", "can", "have", "will", "should") - .desc("the prefixes of fields that return boolean").uiOrder(8.0f).build(); + .desc("the prefixes of fields and variables that indicate boolean").uiOrder(8.0f).build(); public LinguisticNamingRule() { definePropertyDescriptor(CHECK_BOOLEAN_METHODS); @@ -74,29 +76,26 @@ public class LinguisticNamingRule extends AbstractJavaRule { private void checkTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { ASTResultType resultType = node.getResultType(); - if (nameOfMethod.contains("To") && resultType.isVoid()) { - // To in the middle somewhere - // a transform method shouldn't return void linguistically - addViolation(data, node); - } else if (hasPrefix(nameOfMethod, "to") && resultType.isVoid()) { - // a transform method shouldn't return void linguistically - addViolation(data, node); + if (resultType.isVoid() && (containsWord(nameOfMethod, "To") || hasPrefix(nameOfMethod, "to"))) { + // To in the middle somewhere or as prefix + addViolationWithMessage(data, node, "Linguistics Antipattern - The transform method ''{0}'' should not return void linguistically", + new Object[] { nameOfMethod }); } } private void checkGetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { ASTResultType resultType = node.getResultType(); if (hasPrefix(nameOfMethod, "get") && resultType.isVoid()) { - // get method shouldn't return void linguistically - addViolation(data, node); + addViolationWithMessage(data, node, "Linguistics Antipattern - The getter ''{0}'' should not return void linguistically", + new Object[] { nameOfMethod }); } } private void checkSetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { ASTResultType resultType = node.getResultType(); if (hasPrefix(nameOfMethod, "set") && !resultType.isVoid()) { - // set method shouldn't return any type except void linguistically - addViolation(data, node); + addViolationWithMessage(data, node, "Linguistics Antipattern - The setter ''{0}'' should not return any type except void linguistically", + new Object[] { nameOfMethod }); } } @@ -105,38 +104,52 @@ public class LinguisticNamingRule extends AbstractJavaRule { ASTType t = node.getResultType().getFirstChildOfType(ASTType.class); if (!resultType.isVoid() && t != null) { for (String prefix : getProperty(BOOLEAN_METHOD_PREFIXES_PROPERTY)) { - if (hasPrefix(nameOfMethod, prefix) && !"boolean".equals(t.getType().getName())) { - addViolation(data, node); + if (hasPrefix(nameOfMethod, prefix) && !"boolean".equals(t.getTypeImage())) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The method ''{0}'' indicates linguistically it returns a boolean, but it returns ''{1}''", + new Object[] { nameOfMethod, t.getTypeImage() }); } } } } - private void checkField(String nameOfField, Node node, Object data) { - ASTType type = node.getFirstChildOfType(ASTType.class); - if (type != null) { - for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { - if (hasPrefix(nameOfField, prefix) && !"boolean".equals(type.getType().getName())) { - addViolation(data, node); - } + private void checkField(String typeImage, ASTVariableDeclarator node, Object data) { + for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { + if (hasPrefix(node.getName(), prefix) && !"boolean".equals(typeImage)) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The field ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''", + new Object[] { node.getName(), typeImage }); + } + } + } + + private void checkVariable(String typeImage, ASTVariableDeclarator node, Object data) { + for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { + if (hasPrefix(node.getName(), prefix) && !"boolean".equals(typeImage)) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The variable ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''", + new Object[] { node.getName(), typeImage }); } } } @Override public Object visit(ASTFieldDeclaration node, Object data) { - String nameOfField = node.getVariableName(); - if (getProperty(CHECK_FIELDS)) { - checkField(nameOfField, node, data); + ASTType type = node.getFirstChildOfType(ASTType.class); + if (type != null && getProperty(CHECK_FIELDS)) { + List fields = node.findChildrenOfType(ASTVariableDeclarator.class); + for (ASTVariableDeclarator field : fields) { + checkField(type.getTypeImage(), field, data); + } } return data; } @Override public Object visit(ASTLocalVariableDeclaration node, Object data) { - String nameOfField = node.getVariableName(); - if (getProperty(CHECK_VARIABLES)) { - checkField(nameOfField, node, data); + ASTType type = node.getFirstChildOfType(ASTType.class); + if (type != null && getProperty(CHECK_VARIABLES)) { + List variables = node.findChildrenOfType(ASTVariableDeclarator.class); + for (ASTVariableDeclarator variable : variables) { + checkVariable(type.getTypeImage(), variable, data); + } } return data; } @@ -145,4 +158,12 @@ public class LinguisticNamingRule extends AbstractJavaRule { return name.startsWith(prefix) && name.length() > prefix.length() && Character.isUpperCase(name.charAt(prefix.length())); } + + private static boolean containsWord(String name, String word) { + int index = name.indexOf(word); + if (index >= 0 && name.length() > index + word.length()) { + return Character.isUpperCase(name.charAt(index + word.length())); + } + return false; + } } diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 0b5b44dd29..572d97ffb5 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -947,8 +947,10 @@ if (foo) { // preferred approach This rule finds Linguistic Naming Antipatterns. It checks for fields, that are named, as if they should be boolean but have a different type. It also checks for methods, that according to their name, should - return a boolean, but don't. Further, it checks, that getters and transform methods return something, - but setters won't. + return a boolean, but don't. Further, it checks, that getters return something and setters won't. + Finally, it checks that methods, that contain "To" in their name - so called transform methods - + return something, since according to their name, they should convert or transform one object into + another. For more information, see [Linguistic Antipatterns - What They Are and How Developers Perceive Them](https://doi.org/10.1007/s10664-014-9350-8). @@ -957,12 +959,12 @@ Developers Perceive Them](https://doi.org/10.1007/s10664-014-9350-8). Method Prefix is 1 6 + + Linguistics Antipattern - The method 'isValid' indicates linguistically it returns a boolean, but it returns 'int' + Method Prefix Has 1 6 + + Linguistics Antipattern - The method 'hasChild' indicates linguistically it returns a boolean, but it returns 'int' + Method Prefix Have 1 6 + + Linguistics Antipattern - The method 'haveChild' indicates linguistically it returns a boolean, but it returns 'int' + Method Prefix can 1 6 + + Linguistics Antipattern - The method 'canFly' indicates linguistically it returns a boolean, but it returns 'int' + Method Prefix will 1 6 + + Linguistics Antipattern - The method 'willFly' indicates linguistically it returns a boolean, but it returns 'int' + Method Prefix should 1 6 + + Linguistics Antipattern - The method 'shouldFly' indicates linguistically it returns a boolean, but it returns 'long' + Method Setters 1 6 + + Linguistics Antipattern - The setter 'setName' should not return any type except void linguistically + Method Getters 1 6 + + Linguistics Antipattern - The getter 'getName' should not return void linguistically + - Method Prefix to + Method Prefix to: Transform Method 1 6 + + Linguistics Antipattern - The transform method 'toDataType' should not return void linguistically + - Method Contains To + Method Contains To: Transformation methods 1 2 + + Linguistics Antipattern - The transform method 'grapeToWine' should not return void linguistically + @@ -215,6 +248,10 @@ public class MethodTypeAndNameIsInconsistentWithPrefixTo { Field/Variable Prefix is 2 3,8 + + Linguistics Antipattern - The field 'isValid' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'isValidLocal' indicates linguistically it is a boolean, but it is 'int' + Field/Variable Prefix has 2 3,8 + + Linguistics Antipattern - The field 'hasMoney' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'hasMoneyLocal' indicates linguistically it is a boolean, but it is 'int' + Field/Variable Prefix can 2 3,8 + + Linguistics Antipattern - The field 'canFly' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'canFlyLocal' indicates linguistically it is a boolean, but it is 'int' + Field/Variable Prefix will 2 3,8 + + Linguistics Antipattern - The field 'willMove' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'willMoveLocal' indicates linguistically it is a boolean, but it is 'int' + Field/Variable Prefix have 2 3,8 + + Linguistics Antipattern - The field 'haveLegs' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'haveLegsLocal' indicates linguistically it is a boolean, but it is 'int' + Field/Variable Prefix should 2 3,8 + + Linguistics Antipattern - The field 'shouldClimb' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'shouldClimbLocal' indicates linguistically it is a boolean, but it is 'int' + + + + + Multiple fields/local variables + 4 + 2,2,4,4 + From 7d9fd61c500774c0c0a693bff2531da0e6f45185 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Tue, 7 Aug 2018 19:17:21 +0200 Subject: [PATCH 09/12] Fix wrong merge --- docs/pages/release_notes.md | 9 --------- 1 file changed, 9 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 57b8a0c653..f68a7a8d78 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -27,15 +27,6 @@ This is a minor release. Besides method names, the rule also checks field and variable names. It also checks, that getters return something but setters won't. The rule has several properties with which it can be customized. -* The new Java rule [`LocalVariableNamingConventions`](pmd_rules_java_codestyle.html#localvariablenamingconventions) (`java-codestlye`) - detects local variable names that don't comply to a given convention. It defaults to standrd Java convention of using camelCase, - but can be configured. Special cases can be configured for final variables and catched exceptions' names. - -* The new Java rule [`FormalParameterNamingConventions`](pmd_rules_java_codestyle.html#formalparameternamingconventions) (`java-codestlye`) - detects formal parameter names that don't comply to a given convention. It defaults to standrd Java convention of using camelCase, - but can be configured. Special cases can be configured for final parameters and lambda parameters (considering wether they are - explicitly typed or not) - * The new PL/SQL rule [`ForLoopNaming`](pmd_rules_plsql_codestyle.html#forloopnaming) (`plsql-codestyle`) enforces a naming convention for "for loops". Both "cursor for loops" and "index for loops" are covered. The rule can be customized via patterns. By default, short variable names are reported. From 89f027a64f311e557b51f5af94fb506ee58ece87 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Tue, 7 Aug 2018 19:21:56 +0200 Subject: [PATCH 10/12] LinguisticNaming: fix Boolean false-positive --- .../rule/codestyle/LinguisticNamingRule.java | 6 +++--- .../rule/codestyle/xml/LinguisticNaming.xml | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java index 10e4e309f9..941a450e9f 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java @@ -104,7 +104,7 @@ public class LinguisticNamingRule extends AbstractJavaRule { ASTType t = node.getResultType().getFirstChildOfType(ASTType.class); if (!resultType.isVoid() && t != null) { for (String prefix : getProperty(BOOLEAN_METHOD_PREFIXES_PROPERTY)) { - if (hasPrefix(nameOfMethod, prefix) && !"boolean".equals(t.getTypeImage())) { + if (hasPrefix(nameOfMethod, prefix) && !"boolean".equalsIgnoreCase(t.getTypeImage())) { addViolationWithMessage(data, node, "Linguistics Antipattern - The method ''{0}'' indicates linguistically it returns a boolean, but it returns ''{1}''", new Object[] { nameOfMethod, t.getTypeImage() }); } @@ -114,7 +114,7 @@ public class LinguisticNamingRule extends AbstractJavaRule { private void checkField(String typeImage, ASTVariableDeclarator node, Object data) { for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { - if (hasPrefix(node.getName(), prefix) && !"boolean".equals(typeImage)) { + if (hasPrefix(node.getName(), prefix) && !"boolean".equalsIgnoreCase(typeImage)) { addViolationWithMessage(data, node, "Linguistics Antipattern - The field ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''", new Object[] { node.getName(), typeImage }); } @@ -123,7 +123,7 @@ public class LinguisticNamingRule extends AbstractJavaRule { private void checkVariable(String typeImage, ASTVariableDeclarator node, Object data) { for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { - if (hasPrefix(node.getName(), prefix) && !"boolean".equals(typeImage)) { + if (hasPrefix(node.getName(), prefix) && !"boolean".equalsIgnoreCase(typeImage)) { addViolationWithMessage(data, node, "Linguistics Antipattern - The variable ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''", new Object[] { node.getName(), typeImage }); } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml index 8b1288f35f..ba8e176b85 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml @@ -392,6 +392,24 @@ public class MultipleLocalVariables { void myMethod() { int canFly, shouldClimb; } +} + ]]> + + + + Boolean fields/methods false positive + 0 + From 802104525ab35292b79d11586dc966db560bd267 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Tue, 7 Aug 2018 19:32:38 +0200 Subject: [PATCH 11/12] LinguisticNaming: add additional property to avoid too many FPs by default for transform methods --- .../rule/codestyle/LinguisticNamingRule.java | 23 ++++++++++++++++--- .../resources/category/java/codestyle.xml | 8 ++++--- .../rule/codestyle/xml/LinguisticNaming.xml | 1 + 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java index 941a450e9f..7ab22598c0 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java @@ -23,8 +23,11 @@ public class LinguisticNamingRule extends AbstractJavaRule { .desc("Check return type of getters").uiOrder(2.0f).build(); private static final BooleanProperty CHECK_SETTERS = BooleanProperty.named("checkSetters").defaultValue(true) .desc("Check return type of setters").uiOrder(3.0f).build(); + private static final BooleanProperty CHECK_PREFIXED_TRANSFORM_METHODS = BooleanProperty + .named("checkPrefixedTransformMethods") + .defaultValue(true).desc("Check return type of methods whose names start with 'to'").uiOrder(4.0f).build(); private static final BooleanProperty CHECK_TRANSFORM_METHODS = BooleanProperty.named("checkTransformMethods") - .defaultValue(true).desc("Check return type of transform methods").uiOrder(4.0f).build(); + .defaultValue(false).desc("Check return type of methods which contain 'To' in their name").uiOrder(4.0f).build(); private static final StringMultiProperty BOOLEAN_METHOD_PREFIXES_PROPERTY = StringMultiProperty .named("booleanMethodPrefixes").defaultValues("is", "has", "can", "have", "will", "should") .desc("the prefixes of methods that return boolean").uiOrder(5.0f).build(); @@ -41,6 +44,7 @@ public class LinguisticNamingRule extends AbstractJavaRule { definePropertyDescriptor(CHECK_BOOLEAN_METHODS); definePropertyDescriptor(CHECK_GETTERS); definePropertyDescriptor(CHECK_SETTERS); + definePropertyDescriptor(CHECK_PREFIXED_TRANSFORM_METHODS); definePropertyDescriptor(CHECK_TRANSFORM_METHODS); definePropertyDescriptor(BOOLEAN_METHOD_PREFIXES_PROPERTY); definePropertyDescriptor(CHECK_FIELDS); @@ -67,6 +71,10 @@ public class LinguisticNamingRule extends AbstractJavaRule { checkGetters(node, data, nameOfMethod); } + if (getProperty(CHECK_PREFIXED_TRANSFORM_METHODS)) { + checkPrefixedTransformMethods(node, data, nameOfMethod); + } + if (getProperty(CHECK_TRANSFORM_METHODS)) { checkTransformMethods(node, data, nameOfMethod); } @@ -74,10 +82,19 @@ public class LinguisticNamingRule extends AbstractJavaRule { return data; } + private void checkPrefixedTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (resultType.isVoid() && hasPrefix(nameOfMethod, "to")) { + // To as prefix + addViolationWithMessage(data, node, "Linguistics Antipattern - The transform method ''{0}'' should not return void linguistically", + new Object[] { nameOfMethod }); + } + } + private void checkTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { ASTResultType resultType = node.getResultType(); - if (resultType.isVoid() && (containsWord(nameOfMethod, "To") || hasPrefix(nameOfMethod, "to"))) { - // To in the middle somewhere or as prefix + if (resultType.isVoid() && containsWord(nameOfMethod, "To")) { + // To in the middle somewhere addViolationWithMessage(data, node, "Linguistics Antipattern - The transform method ''{0}'' should not return void linguistically", new Object[] { nameOfMethod }); } diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 572d97ffb5..1839646a98 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -948,9 +948,11 @@ if (foo) { // preferred approach This rule finds Linguistic Naming Antipatterns. It checks for fields, that are named, as if they should be boolean but have a different type. It also checks for methods, that according to their name, should return a boolean, but don't. Further, it checks, that getters return something and setters won't. - Finally, it checks that methods, that contain "To" in their name - so called transform methods - - return something, since according to their name, they should convert or transform one object into - another. + Finally, it checks that methods, that start with "to" - so called transform methods - actually return + something, since according to their name, they should convert or transform one object into another. + There is additionally an option, to check for methods that contain "To" in their name - which are + also transform methods. However, this is disabled by default, since this detection is prone to + false positives. For more information, see [Linguistic Antipatterns - What They Are and How Developers Perceive Them](https://doi.org/10.1007/s10664-014-9350-8). diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml index ba8e176b85..68a35fabcf 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/LinguisticNaming.xml @@ -223,6 +223,7 @@ public class MethodTypeAndNameIsInconsistentWithPrefixTo { Method Contains To: Transformation methods + true 1 2 From c3ea6de8e081dadbdbbdd98b975de6f582822713 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 12 Aug 2018 19:25:13 +0200 Subject: [PATCH 12/12] Move LinguisticNaming to 6.7.0 --- pmd-core/src/main/resources/rulesets/releases/660.xml | 1 - pmd-core/src/main/resources/rulesets/releases/670.xml | 2 ++ pmd-java/src/main/resources/category/java/codestyle.xml | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pmd-core/src/main/resources/rulesets/releases/660.xml b/pmd-core/src/main/resources/rulesets/releases/660.xml index f1d5b09f2b..2f84d06b53 100644 --- a/pmd-core/src/main/resources/rulesets/releases/660.xml +++ b/pmd-core/src/main/resources/rulesets/releases/660.xml @@ -9,7 +9,6 @@ This ruleset contains links to rules that are new in PMD v6.6.0 - diff --git a/pmd-core/src/main/resources/rulesets/releases/670.xml b/pmd-core/src/main/resources/rulesets/releases/670.xml index 650eb5c783..6d478e135e 100644 --- a/pmd-core/src/main/resources/rulesets/releases/670.xml +++ b/pmd-core/src/main/resources/rulesets/releases/670.xml @@ -9,5 +9,7 @@ This ruleset contains links to rules that are new in PMD v6.7.0 + + diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 1839646a98..de6d95a69c 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -940,7 +940,7 @@ if (foo) { // preferred approach