From a9be7d4f59504f6eb6d1180b247ce41fd4d2cda2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 20 Aug 2017 00:24:56 +0200 Subject: [PATCH 01/11] Use WMC metric in GodClassRule --- .../lang/java/rule/design/GodClassRule.java | 85 +++---------------- .../main/resources/rulesets/java/design.xml | 5 +- 2 files changed, 15 insertions(+), 75 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java index 585f1587ae..3b519474e2 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java @@ -13,13 +13,7 @@ import java.util.Set; import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; -import net.sourceforge.pmd.lang.java.ast.ASTCatchStatement; -import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; -import net.sourceforge.pmd.lang.java.ast.ASTConditionalAndExpression; -import net.sourceforge.pmd.lang.java.ast.ASTConditionalExpression; -import net.sourceforge.pmd.lang.java.ast.ASTConditionalOrExpression; -import net.sourceforge.pmd.lang.java.ast.ASTForStatement; -import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTLiteral; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclarator; @@ -27,8 +21,8 @@ import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; -import net.sourceforge.pmd.lang.java.ast.ASTSwitchLabel; -import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement; +import net.sourceforge.pmd.lang.java.metrics.JavaMetrics; +import net.sourceforge.pmd.lang.java.metrics.api.JavaClassMetricKey; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.rule.JavaRuleViolation; import net.sourceforge.pmd.lang.java.symboltable.ClassScope; @@ -67,8 +61,6 @@ public class GodClassRule extends AbstractJavaRule { */ private static final double ONE_THIRD_THRESHOLD = 1.0 / 3.0; - /** The Weighted Method Count metric. */ - private int wmcCounter; /** The Access To Foreign Data metric. */ private int atfdCounter; @@ -86,8 +78,9 @@ public class GodClassRule extends AbstractJavaRule { * visited. Afterwards the metrics are evaluated against fixed thresholds. */ @Override - public Object visit(ASTCompilationUnit node, Object data) { - wmcCounter = 0; + public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { + int wmc = (int) JavaMetrics.get(JavaClassMetricKey.WMC, node); + atfdCounter = 0; methodAttributeAccess = new HashMap<>(); @@ -103,14 +96,13 @@ public class GodClassRule extends AbstractJavaRule { // .append("TCC=").append(tcc); // System.out.println(debug.toString()); - if (wmcCounter >= WMC_VERY_HIGH && atfdCounter > FEW_THRESHOLD && tcc < ONE_THIRD_THRESHOLD) { + if (wmc >= WMC_VERY_HIGH && atfdCounter > FEW_THRESHOLD && tcc < ONE_THIRD_THRESHOLD) { - StringBuilder sb = new StringBuilder(); - sb.append(getMessage()).append(" (").append("WMC=").append(wmcCounter).append(", ").append("ATFD=") - .append(atfdCounter).append(", ").append("TCC=").append(tcc).append(')'); + String sb = getMessage() + " (" + "WMC=" + wmc + ", " + "ATFD=" + + atfdCounter + ", " + "TCC=" + tcc + ')'; RuleContext ctx = (RuleContext) data; - ctx.getReport().addRuleViolation(new JavaRuleViolation(this, ctx, node, sb.toString())); + ctx.getReport().addRuleViolation(new JavaRuleViolation(this, ctx, node, sb)); } return result; } @@ -137,11 +129,10 @@ public class GodClassRule extends AbstractJavaRule { * * @return */ - private double calculateTotalMethodPairs() { + private int calculateTotalMethodPairs() { int methodCount = methodAttributeAccess.size(); int n = methodCount - 1; - double totalMethodPairs = n * (n + 1) / 2.0; - return totalMethodPairs; + return n * (n + 1) / 2; } /** @@ -303,8 +294,6 @@ public class GodClassRule extends AbstractJavaRule { @Override public Object visit(ASTMethodDeclaration node, Object data) { - wmcCounter++; - currentMethodName = node.getFirstChildOfType(ASTMethodDeclarator.class).getImage(); methodAttributeAccess.put(currentMethodName, new HashSet()); @@ -315,54 +304,4 @@ public class GodClassRule extends AbstractJavaRule { return result; } - @Override - public Object visit(ASTConditionalOrExpression node, Object data) { - wmcCounter++; - return super.visit(node, data); - } - - @Override - public Object visit(ASTConditionalAndExpression node, Object data) { - wmcCounter++; - return super.visit(node, data); - } - - @Override - public Object visit(ASTIfStatement node, Object data) { - wmcCounter++; - return super.visit(node, data); - } - - @Override - public Object visit(ASTWhileStatement node, Object data) { - wmcCounter++; - return super.visit(node, data); - } - - @Override - public Object visit(ASTForStatement node, Object data) { - wmcCounter++; - return super.visit(node, data); - } - - @Override - public Object visit(ASTSwitchLabel node, Object data) { - wmcCounter++; - return super.visit(node, data); - } - - @Override - public Object visit(ASTCatchStatement node, Object data) { - wmcCounter++; - return super.visit(node, data); - } - - @Override - public Object visit(ASTConditionalExpression node, Object data) { - if (node.isTernary()) { - wmcCounter++; - } - return super.visit(node, data); - } - } diff --git a/pmd-java/src/main/resources/rulesets/java/design.xml b/pmd-java/src/main/resources/rulesets/java/design.xml index 148bf96ee5..49d09261c7 100644 --- a/pmd-java/src/main/resources/rulesets/java/design.xml +++ b/pmd-java/src/main/resources/rulesets/java/design.xml @@ -1,9 +1,9 @@ + xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/3.0.0 http://pmd.sourceforge.net/ruleset_3_0_0.xsd"> The Design ruleset contains rules that flag suboptimal code implementations. Alternate approaches @@ -1857,6 +1857,7 @@ public class HelloWorldBean { language="java" since="5.0" message="Possible God class" + metrics="true" class="net.sourceforge.pmd.lang.java.rule.design.GodClassRule" externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_design.html#godclass"> From 6a6804c7a142fb79a4317ae981db836e4509ed34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 20 Aug 2017 00:27:06 +0200 Subject: [PATCH 02/11] TCC metric --- .../ast/JavaParserVisitorReducedAdapter.java | 10 +- .../java/metrics/api/JavaClassMetricKey.java | 12 +- .../pmd/lang/java/metrics/impl/TccMetric.java | 80 ++++++++++++ .../impl/visitors/TccMethodPairVisitor.java | 121 ++++++++++++++++++ .../java/metrics/impl/AllMetricsTest.java | 1 + .../lang/java/metrics/impl/TccTestRule.java | 26 ++++ .../lang/java/metrics/impl/xml/TccTest.xml | 100 +++++++++++++++ .../resources/rulesets/java/metrics_test.xml | 6 + 8 files changed, 349 insertions(+), 7 deletions(-) create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccMethodPairVisitor.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/TccTestRule.java create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaParserVisitorReducedAdapter.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaParserVisitorReducedAdapter.java index a997230f5e..90eb457089 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaParserVisitorReducedAdapter.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaParserVisitorReducedAdapter.java @@ -12,19 +12,19 @@ package net.sourceforge.pmd.lang.java.ast; public class JavaParserVisitorReducedAdapter extends JavaParserVisitorAdapter { @Override - public final Object visit(ASTClassOrInterfaceDeclaration node, Object data) { + public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { return visit((ASTAnyTypeDeclaration) node, data); } @Override - public final Object visit(ASTAnnotationTypeDeclaration node, Object data) { + public Object visit(ASTAnnotationTypeDeclaration node, Object data) { return visit((ASTAnyTypeDeclaration) node, data); } @Override - public final Object visit(ASTEnumDeclaration node, Object data) { + public Object visit(ASTEnumDeclaration node, Object data) { return visit((ASTAnyTypeDeclaration) node, data); } @@ -35,13 +35,13 @@ public class JavaParserVisitorReducedAdapter extends JavaParserVisitorAdapter { @Override - public final Object visit(ASTMethodDeclaration node, Object data) { + public Object visit(ASTMethodDeclaration node, Object data) { return visit((ASTMethodOrConstructorDeclaration) node, data); } @Override - public final Object visit(ASTConstructorDeclaration node, Object data) { + public Object visit(ASTConstructorDeclaration node, Object data) { return visit((ASTMethodOrConstructorDeclaration) node, data); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/api/JavaClassMetricKey.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/api/JavaClassMetricKey.java index 9ef5fd0ed2..2d51621aa1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/api/JavaClassMetricKey.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/api/JavaClassMetricKey.java @@ -10,6 +10,7 @@ import net.sourceforge.pmd.lang.java.metrics.impl.LocMetric.LocClassMetric; import net.sourceforge.pmd.lang.java.metrics.impl.NcssMetric.NcssClassMetric; import net.sourceforge.pmd.lang.java.metrics.impl.NoamMetric; import net.sourceforge.pmd.lang.java.metrics.impl.NopaMetric; +import net.sourceforge.pmd.lang.java.metrics.impl.TccMetric; import net.sourceforge.pmd.lang.java.metrics.impl.WmcMetric; import net.sourceforge.pmd.lang.java.metrics.impl.WocMetric; import net.sourceforge.pmd.lang.metrics.MetricKey; @@ -60,13 +61,20 @@ public enum JavaClassMetricKey implements MetricKey { * @see NopaMetric */ NOAM(new NoamMetric()), - + /** * Weight of class. * * @see WocMetric */ - WOC(new WocMetric()); + WOC(new WocMetric()), + + /** + * Tight Class Cohesion. + * + * @see TccMetric + */ + TCC(new TccMetric()); private final JavaClassMetric calculator; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java new file mode 100644 index 0000000000..e5f40398c9 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java @@ -0,0 +1,80 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.metrics.impl; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; +import net.sourceforge.pmd.lang.java.metrics.impl.visitors.TccMethodPairVisitor; +import net.sourceforge.pmd.lang.metrics.MetricOptions; + +/** + * Tight class cohesion. + * + * @author Clément Fournier + * @since 6.0.0 + */ +public class TccMetric extends AbstractJavaClassMetric { + + + @Override + public double computeFor(ASTAnyTypeDeclaration node, MetricOptions options) { + @SuppressWarnings("unchecked") + Map> usagesByMethod = (Map>) node.jjtAccept(new TccMethodPairVisitor(), null); + + int numPairs = numMethodsRelatedByAttributeAccess(usagesByMethod); + int maxPairs = maxMethodPairs(usagesByMethod.size()); + + double tcc = maxPairs == 0 ? 0. : numPairs / (double) maxPairs; + + return tcc; + } + + + /** + * Gets the number of pairs of methods that use at least one attribute in common. + * + * @param usagesByMethod Map of method name to names of local attributes accessed + * + * @return The number of pairs + */ + private int numMethodsRelatedByAttributeAccess(Map> usagesByMethod) { + List methods = new ArrayList<>(usagesByMethod.keySet()); + int methodCount = methods.size(); + int pairs = 0; + + if (methodCount > 1) { + for (int i = 0; i < methodCount; i++) { + for (int j = i + 1; j < methodCount; j++) { + String firstMethodName = methods.get(i); + String secondMethodName = methods.get(j); + + if (!Collections.disjoint(usagesByMethod.get(firstMethodName), + usagesByMethod.get(secondMethodName))) { + pairs++; + } + } + } + } + return pairs; + } + + + /** + * Calculates the number of possible method pairs of two methods. + * + * @param methods Number of methods in the class + * + * @return Number of possible method pairs + */ + private int maxMethodPairs(int methods) { + return methods * (methods - 1) / 2; + } + +} diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccMethodPairVisitor.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccMethodPairVisitor.java new file mode 100644 index 0000000000..e804812656 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/TccMethodPairVisitor.java @@ -0,0 +1,121 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.metrics.impl.visitors; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.Stack; + +import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTName; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; +import net.sourceforge.pmd.lang.java.ast.JavaParserVisitorReducedAdapter; +import net.sourceforge.pmd.lang.java.symboltable.ClassScope; +import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; +import net.sourceforge.pmd.lang.symboltable.Scope; + +/** + * Returns the map of method names to the set local attributes accessed when visiting a class. + * + * @author Clément Fournier + * @since 6.0.0 + */ +public class TccMethodPairVisitor extends JavaParserVisitorReducedAdapter { + + /** + * Collects for each method of the current class, which local attributes are accessed. + */ + Stack>> methodAttributeAccess = new Stack<>(); + + /** The name of the current method. */ + private String currentMethodName; + + + @Override + public Object visit(ASTAnyTypeDeclaration node, Object data) { + methodAttributeAccess.push(new HashMap>()); + super.visit(node, data); + + methodAttributeAccess.peek().remove(null); + return methodAttributeAccess.pop(); + } + + + @Override + public Object visit(ASTMethodDeclaration node, Object data) { + + if (!node.isAbstract()) { + currentMethodName = node.getQualifiedName().getOperation(); + methodAttributeAccess.peek().put(currentMethodName, new HashSet()); + + super.visit(node, data); + + currentMethodName = null; + } + + return null; + } + + + /** + * The primary expression node is used to detect access to attributes and method calls. If the access is not for a + * foreign class, then the {@link #methodAttributeAccess} map is updated for the current method. + */ + @Override + public Object visit(ASTPrimaryExpression node, Object data) { + if (currentMethodName != null) { + Set methodAccess = methodAttributeAccess.peek().get(currentMethodName); + String variableName = getVariableName(node); + if (isLocalAttributeAccess(variableName, node.getScope())) { + methodAccess.add(variableName); + } + } + + + return super.visit(node, data); + } + + + private String getVariableName(ASTPrimaryExpression node) { + ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); + ASTName name = prefix.getFirstDescendantOfType(ASTName.class); + + String variableName = null; + + if (name != null) { + int dotIndex = name.getImage().indexOf("."); + if (dotIndex == -1) { + variableName = name.getImage(); + } else { + variableName = name.getImage().substring(0, dotIndex); + } + } + + return variableName; + } + + + private boolean isLocalAttributeAccess(String varName, Scope scope) { + Scope currentScope = scope; + + while (currentScope != null) { + for (VariableNameDeclaration decl : currentScope.getDeclarations(VariableNameDeclaration.class).keySet()) { + if (decl.getImage().equals(varName)) { + if (currentScope instanceof ClassScope) { + return true; + } + } + } + currentScope = currentScope.getParent(); // WARNING doesn't consider inherited fields or static imports + } + + return false; + } + +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java index 3ae9def4a4..aa7756200b 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java @@ -36,6 +36,7 @@ public class AllMetricsTest extends SimpleAggregatorTst { addRule(RULESET, "NopaTest"); addRule(RULESET, "NoamTest"); addRule(RULESET, "WocTest"); + addRule(RULESET, "TccTest"); } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/TccTestRule.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/TccTestRule.java new file mode 100644 index 0000000000..2b478a1a44 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/TccTestRule.java @@ -0,0 +1,26 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.metrics.impl; + +import net.sourceforge.pmd.lang.java.metrics.api.JavaClassMetricKey; +import net.sourceforge.pmd.lang.java.metrics.api.JavaOperationMetricKey; + +/** + * @author Clément Fournier + * @since 6.0.0 + */ +public class TccTestRule extends AbstractMetricTestRule { + + @Override + protected JavaClassMetricKey getClassKey() { + return JavaClassMetricKey.TCC; + } + + + @Override + protected JavaOperationMetricKey getOpKey() { + return null; + } +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml new file mode 100644 index 0000000000..fb02141dcb --- /dev/null +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/TccTest.xml @@ -0,0 +1,100 @@ + + + + + + + + Full example + 1 + + '.Property' has value 9.52%. + + + + + + Test empty class + 1 + + '.Foo' has value 0. + + + + + + TCC doesn't support interfaces or annotations + 0 + + + + diff --git a/pmd-java/src/test/resources/rulesets/java/metrics_test.xml b/pmd-java/src/test/resources/rulesets/java/metrics_test.xml index e6424a61a7..258f83f764 100644 --- a/pmd-java/src/test/resources/rulesets/java/metrics_test.xml +++ b/pmd-java/src/test/resources/rulesets/java/metrics_test.xml @@ -57,4 +57,10 @@ metrics="true"> + + + From d14603078d8b76ff54767f37eb590b435c437485 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 20 Aug 2017 01:10:42 +0200 Subject: [PATCH 03/11] Use TCC metric in GodClassRule --- .../net/sourceforge/pmd/util/StringUtil.java | 42 +++++ .../lang/java/rule/design/GodClassRule.java | 173 ++---------------- .../metrics/impl/AbstractMetricTestRule.java | 31 +--- 3 files changed, 70 insertions(+), 176 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java index 375892308a..59f012edad 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java @@ -27,6 +27,48 @@ public final class StringUtil { } + /** + * Formats a double to a percentage, keeping {@code numDecimal} decimal places. + * + * @param val a double value between 0 and 1 + * @param numDecimals The number of decimal places to keep + * @param truncateInt Whether to truncate the string to an int if possible (if this is set, then 10.0% will be + * represented as "10%") + * + * @return A formatted string + * + * @throws IllegalArgumentException if the double to format is not between 0 and 1 + */ + public static String percentageString(double val, int numDecimals, boolean truncateInt) { + if (val < 0 || val > 1) { + throw new IllegalArgumentException("Expected a number between 0 and 1"); + } + + return truncateDouble(100 * val, numDecimals, truncateInt) + "%"; + } + + + /** + * Returns a string representation of the double {@code val}, truncated to {@code numDecimal} decimal places. + * + * @param val The value to present + * @param numDecimals The number of decimal places to keep + * @param truncateInt Whether to truncate the string to an int if possible (if this is set, then 10.0 will be + * represented as "10") + * + * @return A string representation of the double number + */ + public static String truncateDouble(double val, int numDecimals, boolean truncateInt) { + int factor = (int) Math.pow(10, numDecimals); + double truncated = Math.floor(factor * val) / factor; + + if (truncateInt && truncated == Math.floor(truncated)) { + return String.valueOf((int) truncated); + } else { + return String.valueOf(truncated); + } + } + /** * Return whether the non-null text arg starts with any of the prefix * values. diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java index 3b519474e2..7e28760832 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java @@ -4,19 +4,12 @@ package net.sourceforge.pmd.lang.java.rule.design; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.HashSet; import java.util.List; -import java.util.Map; -import java.util.Set; import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTLiteral; -import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclarator; import net.sourceforge.pmd.lang.java.ast.ASTName; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; @@ -25,68 +18,53 @@ import net.sourceforge.pmd.lang.java.metrics.JavaMetrics; import net.sourceforge.pmd.lang.java.metrics.api.JavaClassMetricKey; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.rule.JavaRuleViolation; -import net.sourceforge.pmd.lang.java.symboltable.ClassScope; -import net.sourceforge.pmd.lang.java.symboltable.SourceFileScope; -import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; -import net.sourceforge.pmd.lang.symboltable.Scope; import net.sourceforge.pmd.util.StringUtil; /** - * The God Class Rule detects a the God Class design flaw using metrics. A god - * class does too many things, is very big and complex. It should be split apart - * to be more object-oriented. The rule uses the detection strategy described in + * The God Class Rule detects a the God Class design flaw using metrics. A god class does too many things, is very big + * and complex. It should be split apart to be more object-oriented. The rule uses the detection strategy described in * [1]. The violations are reported against the entire class. - * + * * [1] Lanza. Object-Oriented Metrics in Practice. Page 80. - * + * * @since 5.0 */ public class GodClassRule extends AbstractJavaRule { /** - * Very high threshold for WMC (Weighted Method Count). See: Lanza. - * Object-Oriented Metrics in Practice. Page 16. + * Very high threshold for WMC (Weighted Method Count). See: Lanza. Object-Oriented Metrics in Practice. Page 16. */ private static final int WMC_VERY_HIGH = 47; /** - * Few means between 2 and 5. See: Lanza. Object-Oriented Metrics in - * Practice. Page 18. + * Few means between 2 and 5. See: Lanza. Object-Oriented Metrics in Practice. Page 18. */ private static final int FEW_THRESHOLD = 5; /** - * One third is a low value. See: Lanza. Object-Oriented Metrics in - * Practice. Page 17. + * One third is a low value. See: Lanza. Object-Oriented Metrics in Practice. Page 17. */ - private static final double ONE_THIRD_THRESHOLD = 1.0 / 3.0; + private static final double TCC_THRESHOLD = 1.0 / 3.0; + /** The Access To Foreign Data metric. */ private int atfdCounter; - /** - * Collects for each method of the current class, which local attributes are - * accessed. - */ - private Map> methodAttributeAccess; - /** The name of the current method. */ - private String currentMethodName; /** - * Base entry point for the visitor - the compilation unit (everything - * within one file). The metrics are initialized. Then the other nodes are - * visited. Afterwards the metrics are evaluated against fixed thresholds. + * Base entry point for the visitor - the compilation unit (everything within one file). The metrics are + * initialized. Then the other nodes are visited. Afterwards the metrics are evaluated against fixed thresholds. */ @Override public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { int wmc = (int) JavaMetrics.get(JavaClassMetricKey.WMC, node); + double tcc = JavaMetrics.get(JavaClassMetricKey.TCC, node); + atfdCounter = 0; - methodAttributeAccess = new HashMap<>(); Object result = super.visit(node, data); - double tcc = calculateTcc(); // StringBuilder debug = new StringBuilder(); // debug.append("Values for class ") @@ -96,7 +74,7 @@ public class GodClassRule extends AbstractJavaRule { // .append("TCC=").append(tcc); // System.out.println(debug.toString()); - if (wmc >= WMC_VERY_HIGH && atfdCounter > FEW_THRESHOLD && tcc < ONE_THIRD_THRESHOLD) { + if (wmc >= WMC_VERY_HIGH && atfdCounter > FEW_THRESHOLD && tcc < TCC_THRESHOLD) { String sb = getMessage() + " (" + "WMC=" + wmc + ", " + "ATFD=" + atfdCounter + ", " + "TCC=" + tcc + ')'; @@ -107,92 +85,18 @@ public class GodClassRule extends AbstractJavaRule { return result; } - /** - * Calculates the Tight Class Cohesion metric. - * - * @return a value between 0 and 1. - */ - private double calculateTcc() { - double tcc = 0.0; - int methodPairs = determineMethodPairs(); - double totalMethodPairs = calculateTotalMethodPairs(); - if (totalMethodPairs > 0) { - tcc = methodPairs / totalMethodPairs; - } - return tcc; - } - /** - * Calculates the number of possible method pairs. Its basically the sum of - * the first (methodCount - 1) integers. It will be 0, if no methods exist - * or only one method, means, if no pairs exist. - * - * @return - */ - private int calculateTotalMethodPairs() { - int methodCount = methodAttributeAccess.size(); - int n = methodCount - 1; - return n * (n + 1) / 2; - } - - /** - * Uses the {@link #methodAttributeAccess} map to detect method pairs, that - * use at least one common attribute of the class. - * - * @return - */ - private int determineMethodPairs() { - List methods = new ArrayList<>(methodAttributeAccess.keySet()); - int methodCount = methods.size(); - int pairs = 0; - - if (methodCount > 1) { - for (int i = 0; i < methodCount; i++) { - for (int j = i + 1; j < methodCount; j++) { - String firstMethodName = methods.get(i); - String secondMethodName = methods.get(j); - Set accessesOfFirstMethod = methodAttributeAccess.get(firstMethodName); - Set accessesOfSecondMethod = methodAttributeAccess.get(secondMethodName); - Set combinedAccesses = new HashSet<>(); - - combinedAccesses.addAll(accessesOfFirstMethod); - combinedAccesses.addAll(accessesOfSecondMethod); - - if (combinedAccesses.size() < (accessesOfFirstMethod.size() + accessesOfSecondMethod.size())) { - pairs++; - } - } - } - } - return pairs; - } - - /** - * The primary expression node is used to detect access to attributes and - * method calls. If the access is not for a foreign class, then the - * {@link #methodAttributeAccess} map is updated for the current method. - */ @Override public Object visit(ASTPrimaryExpression node, Object data) { if (isForeignAttributeOrMethod(node)) { if (isAttributeAccess(node) || isMethodCall(node) && isForeignGetterSetterCall(node)) { atfdCounter++; } - } else { - if (currentMethodName != null) { - Set methodAccess = methodAttributeAccess.get(currentMethodName); - String variableName = getVariableName(node); - VariableNameDeclaration variableDeclaration = findVariableDeclaration(variableName, - node.getScope().getEnclosingScope(ClassScope.class)); - if (variableDeclaration != null) { - methodAccess.add(variableName); - } - } } - return super.visit(node, data); } + private boolean isForeignGetterSetterCall(ASTPrimaryExpression node) { String methodOrAttributeName = getMethodOrAttributeName(node); @@ -200,6 +104,7 @@ public class GodClassRule extends AbstractJavaRule { return methodOrAttributeName != null && StringUtil.startsWithAny(methodOrAttributeName, "get", "is", "set"); } + private boolean isMethodCall(ASTPrimaryExpression node) { boolean result = false; List suffixes = node.findDescendantsOfType(ASTPrimarySuffix.class); @@ -209,6 +114,7 @@ public class GodClassRule extends AbstractJavaRule { return result; } + private boolean isForeignAttributeOrMethod(ASTPrimaryExpression node) { boolean result = false; String nameImage = getNameImage(node); @@ -226,6 +132,7 @@ public class GodClassRule extends AbstractJavaRule { return result; } + private String getNameImage(ASTPrimaryExpression node) { ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); ASTName name = prefix.getFirstDescendantOfType(ASTName.class); @@ -237,23 +144,6 @@ public class GodClassRule extends AbstractJavaRule { return image; } - private String getVariableName(ASTPrimaryExpression node) { - ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); - ASTName name = prefix.getFirstDescendantOfType(ASTName.class); - - String variableName = null; - - if (name != null) { - int dotIndex = name.getImage().indexOf("."); - if (dotIndex == -1) { - variableName = name.getImage(); - } else { - variableName = name.getImage().substring(0, dotIndex); - } - } - - return variableName; - } private String getMethodOrAttributeName(ASTPrimaryExpression node) { ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); @@ -271,37 +161,10 @@ public class GodClassRule extends AbstractJavaRule { return methodOrAttributeName; } - private VariableNameDeclaration findVariableDeclaration(String variableName, Scope scope) { - VariableNameDeclaration result = null; - - for (VariableNameDeclaration declaration : scope.getDeclarations(VariableNameDeclaration.class).keySet()) { - if (declaration.getImage().equals(variableName)) { - result = declaration; - break; - } - } - - if (result == null && scope.getParent() != null && !(scope.getParent() instanceof SourceFileScope)) { - result = findVariableDeclaration(variableName, scope.getParent()); - } - - return result; - } private boolean isAttributeAccess(ASTPrimaryExpression node) { return node.findDescendantsOfType(ASTPrimarySuffix.class).isEmpty(); } - @Override - public Object visit(ASTMethodDeclaration node, Object data) { - currentMethodName = node.getFirstChildOfType(ASTMethodDeclarator.class).getImage(); - methodAttributeAccess.put(currentMethodName, new HashSet()); - - Object result = super.visit(node, data); - - currentMethodName = null; - - return result; - } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java index 6092bd905a..909e8767fd 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java @@ -21,6 +21,7 @@ import net.sourceforge.pmd.lang.metrics.ResultOption; import net.sourceforge.pmd.lang.rule.properties.BooleanProperty; import net.sourceforge.pmd.lang.rule.properties.DoubleProperty; import net.sourceforge.pmd.lang.rule.properties.EnumeratedMultiProperty; +import net.sourceforge.pmd.util.StringUtil; /** * Abstract test rule for a metric. Tests of metrics use the standard framework for rule testing, using one dummy rule @@ -126,26 +127,14 @@ public abstract class AbstractMetricTestRule extends AbstractJavaMetricsRule { } - /** Gets a string representation rounded to the nearest half. */ - private String presentableString(double val) { - boolean isInt = Math.floor(val) == val; + /** Gets a nice string representation of a double. */ + public String niceDoubleString(double val) { + int numDecimalPlaces = 2; - if (!isInt && val >= 0 && val <= 1) { // percentage - return roundedString(100 * val) + "%"; - } else if (!isInt) { - return String.valueOf(roundedString(val)); + if (val > 0 && val < 1) { // percentage + return StringUtil.percentageString(val, numDecimalPlaces, true); } else { - return String.valueOf((int) val); - } - } - - - private String roundedString(double val) { - double truncated = Math.floor(100 * val) / 100; - if (truncated == Math.floor(truncated)) { - return String.valueOf((int) truncated); - } else { - return String.valueOf(truncated); + return String.valueOf(StringUtil.truncateDouble(val, numDecimalPlaces, true)); } } @@ -155,11 +144,11 @@ public abstract class AbstractMetricTestRule extends AbstractJavaMetricsRule { if (classKey != null && reportClasses && classKey.supports(node)) { double classValue = JavaMetrics.get(classKey, node, metricOptions); - String valueReport = presentableString(classValue); + String valueReport = niceDoubleString(classValue); if (opKey != null) { double highest = JavaMetrics.get(opKey, node, metricOptions, ResultOption.HIGHEST); - valueReport += " highest " + presentableString(highest); + valueReport += " highest " + niceDoubleString(highest); } if (classValue >= reportLevel) { addViolation(data, node, new String[] {node.getQualifiedName().toString(), valueReport, }); @@ -175,7 +164,7 @@ public abstract class AbstractMetricTestRule extends AbstractJavaMetricsRule { double methodValue = JavaMetrics.get(opKey, node, metricOptions); if (methodValue >= reportLevel) { addViolation(data, node, new String[] {node.getQualifiedName().toString(), - "" + presentableString(methodValue), }); + "" + niceDoubleString(methodValue), }); } } return data; From 2f678791ccee6d0bebf67e7fa12eb985653df2e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 20 Aug 2017 01:40:11 +0200 Subject: [PATCH 04/11] ATFD metric --- .../lang/java/metrics/impl/AtfdMetric.java | 32 +-- .../impl/visitors/AtfdBaseVisitor.java | 109 ++++++++++ .../java/metrics/impl/AllMetricsTest.java | 1 + .../lang/java/metrics/impl/AtfdTestRule.java | 26 +++ .../lang/java/metrics/impl/xml/AtfdTest.xml | 186 ++++++++++++++++++ .../resources/rulesets/java/metrics_test.xml | 6 + 6 files changed, 338 insertions(+), 22 deletions(-) create mode 100644 pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdTestRule.java create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java index 9f15a84271..946c13ef78 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java @@ -4,15 +4,15 @@ package net.sourceforge.pmd.lang.java.metrics.impl; -import java.util.List; +import org.apache.commons.lang3.mutable.MutableInt; import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration; -import net.sourceforge.pmd.lang.java.ast.JavaQualifiedName; -import net.sourceforge.pmd.lang.java.metrics.signature.JavaOperationSigMask; -import net.sourceforge.pmd.lang.java.metrics.signature.JavaOperationSignature.Role; -import net.sourceforge.pmd.lang.java.metrics.signature.JavaSignature.Visibility; +import net.sourceforge.pmd.lang.java.metrics.JavaMetrics; +import net.sourceforge.pmd.lang.java.metrics.api.JavaOperationMetricKey; +import net.sourceforge.pmd.lang.java.metrics.impl.visitors.AtfdBaseVisitor; import net.sourceforge.pmd.lang.metrics.MetricOptions; +import net.sourceforge.pmd.lang.metrics.ResultOption; /** * Access to Foreign Data. Quantifies the number of foreign fields accessed directly or via accessors. @@ -24,31 +24,19 @@ public final class AtfdMetric { public static final class AtfdOperationMetric extends AbstractJavaOperationMetric { - @Override // TODO:cf + @Override public double computeFor(ASTMethodOrConstructorDeclaration node, MetricOptions options) { - - JavaOperationSigMask targetOps = new JavaOperationSigMask(); - targetOps.restrictVisibilitiesTo(Visibility.PUBLIC); - targetOps.restrictRolesTo(Role.GETTER_OR_SETTER); - - List callQNames = findAllCalls(node); - int foreignCalls = 0; - for (JavaQualifiedName name : callQNames) { - if (getSignatureMatcher().hasMatchingSig(name, targetOps)) { - foreignCalls++; - } - } - - return foreignCalls / callQNames.size(); + return ((MutableInt) node.jjtAccept(new AtfdBaseVisitor(), new MutableInt(0))).getValue(); } + } public static final class AtfdClassMetric extends AbstractJavaClassMetric { @Override public double computeFor(ASTAnyTypeDeclaration node, MetricOptions options) { - // TODO:cf - return 0; + // TODO maybe consider code outside methods + return JavaMetrics.get(JavaOperationMetricKey.ATFD, node, options, ResultOption.SUM); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java new file mode 100644 index 0000000000..77b32dd102 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/AtfdBaseVisitor.java @@ -0,0 +1,109 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.metrics.impl.visitors; + +import java.util.List; + +import org.apache.commons.lang3.mutable.MutableInt; + +import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; +import net.sourceforge.pmd.lang.java.ast.ASTLiteral; +import net.sourceforge.pmd.lang.java.ast.ASTName; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; +import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; +import net.sourceforge.pmd.lang.java.ast.JavaParserVisitorAdapter; +import net.sourceforge.pmd.util.StringUtil; + +/** + * Computes Atfd. + * + * @author Clément Fournier + * @since 6.0.0 + */ +public class AtfdBaseVisitor extends JavaParserVisitorAdapter { + + + @Override + public Object visit(ASTPrimaryExpression node, Object data) { + if (isForeignAttributeOrMethod(node) && (isAttributeAccess(node) + || isMethodCall(node) && isForeignGetterSetterCall(node))) { + + ((MutableInt) data).increment(); + } + return super.visit(node, data); + } + + + private boolean isForeignGetterSetterCall(ASTPrimaryExpression node) { + + String methodOrAttributeName = getMethodOrAttributeName(node); + + return methodOrAttributeName != null && StringUtil.startsWithAny(methodOrAttributeName, "get", "is", "set"); + } + + + private boolean isMethodCall(ASTPrimaryExpression node) { + boolean result = false; + List suffixes = node.findDescendantsOfType(ASTPrimarySuffix.class); + if (suffixes.size() == 1) { + result = suffixes.get(0).isArguments(); + } + return result; + } + + + private boolean isForeignAttributeOrMethod(ASTPrimaryExpression node) { + boolean result; + String nameImage = getNameImage(node); + + if (nameImage != null && (!nameImage.contains(".") || nameImage.startsWith("this."))) { + result = false; + } else if (nameImage == null && node.getFirstDescendantOfType(ASTPrimaryPrefix.class).usesThisModifier()) { + result = false; + } else if (nameImage == null && node.hasDecendantOfAnyType(ASTLiteral.class, ASTAllocationExpression.class)) { + result = false; + } else { + result = true; + } + + return result; + } + + + private String getNameImage(ASTPrimaryExpression node) { + ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); + ASTName name = prefix.getFirstDescendantOfType(ASTName.class); + + String image = null; + if (name != null) { + image = name.getImage(); + } + return image; + } + + + private String getMethodOrAttributeName(ASTPrimaryExpression node) { + ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); + ASTName name = prefix.getFirstDescendantOfType(ASTName.class); + + String methodOrAttributeName = null; + + if (name != null) { + int dotIndex = name.getImage().indexOf("."); + if (dotIndex > -1) { + methodOrAttributeName = name.getImage().substring(dotIndex + 1); + } + } + + return methodOrAttributeName; + } + + + private boolean isAttributeAccess(ASTPrimaryExpression node) { + return node.findDescendantsOfType(ASTPrimarySuffix.class).isEmpty(); + } + +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java index aa7756200b..f5d3b6e2a9 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AllMetricsTest.java @@ -37,6 +37,7 @@ public class AllMetricsTest extends SimpleAggregatorTst { addRule(RULESET, "NoamTest"); addRule(RULESET, "WocTest"); addRule(RULESET, "TccTest"); + addRule(RULESET, "AtfdTest"); } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdTestRule.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdTestRule.java new file mode 100644 index 0000000000..11a17bb4a4 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdTestRule.java @@ -0,0 +1,26 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.metrics.impl; + +import net.sourceforge.pmd.lang.java.metrics.api.JavaClassMetricKey; +import net.sourceforge.pmd.lang.java.metrics.api.JavaOperationMetricKey; + +/** + * @author Clément Fournier + * @since 6.0.0 + */ +public class AtfdTestRule extends AbstractMetricTestRule { + + @Override + protected JavaClassMetricKey getClassKey() { + return JavaClassMetricKey.ATFD; + } + + + @Override + protected JavaOperationMetricKey getOpKey() { + return JavaOperationMetricKey.ATFD; + } +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml new file mode 100644 index 0000000000..6a060ba0ce --- /dev/null +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml @@ -0,0 +1,186 @@ + + + + + + + + Full example + 1 + + '.StatementAndBraceFinder' has value 10. + + + + + + Test empty class + 1 + false + + '.Foo' has value 0. + + + + + + Test empty class + + 1 + + '.Foo' has value 0. + + + + + + NOPA doesn't support enums, interfaces or annotations + 0 + + + + \ No newline at end of file diff --git a/pmd-java/src/test/resources/rulesets/java/metrics_test.xml b/pmd-java/src/test/resources/rulesets/java/metrics_test.xml index 258f83f764..be4470c5a5 100644 --- a/pmd-java/src/test/resources/rulesets/java/metrics_test.xml +++ b/pmd-java/src/test/resources/rulesets/java/metrics_test.xml @@ -63,4 +63,10 @@ metrics="true"> + + + From 5001701989a7070d0bc5d1117bb40bdf5240c91b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sun, 20 Aug 2017 02:59:42 +0200 Subject: [PATCH 05/11] Use ATFD metric in GodClassRule -> done refactoring --- .../net/sourceforge/pmd/util/StringUtil.java | 2 +- .../lang/java/rule/design/GodClassRule.java | 133 ++---------------- .../main/resources/rulesets/java/design.xml | 2 +- 3 files changed, 12 insertions(+), 125 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java index 59f012edad..29318082dc 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java @@ -61,7 +61,7 @@ public final class StringUtil { public static String truncateDouble(double val, int numDecimals, boolean truncateInt) { int factor = (int) Math.pow(10, numDecimals); double truncated = Math.floor(factor * val) / factor; - + if (truncateInt && truncated == Math.floor(truncated)) { return String.valueOf((int) truncated); } else { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java index 7e28760832..ef90be10bf 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java @@ -4,28 +4,18 @@ package net.sourceforge.pmd.lang.java.rule.design; -import java.util.List; - -import net.sourceforge.pmd.RuleContext; -import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration; -import net.sourceforge.pmd.lang.java.ast.ASTLiteral; -import net.sourceforge.pmd.lang.java.ast.ASTName; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; -import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; import net.sourceforge.pmd.lang.java.metrics.JavaMetrics; import net.sourceforge.pmd.lang.java.metrics.api.JavaClassMetricKey; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.lang.java.rule.JavaRuleViolation; import net.sourceforge.pmd.util.StringUtil; /** - * The God Class Rule detects a the God Class design flaw using metrics. A god class does too many things, is very big + * The God Class Rule detects the God Class design flaw using metrics. A god class does too many things, is very big * and complex. It should be split apart to be more object-oriented. The rule uses the detection strategy described in * [1]. The violations are reported against the entire class. * - * [1] Lanza. Object-Oriented Metrics in Practice. Page 80. + *

[1] Lanza. Object-Oriented Metrics in Practice. Page 80. * * @since 5.0 */ @@ -39,7 +29,7 @@ public class GodClassRule extends AbstractJavaRule { /** * Few means between 2 and 5. See: Lanza. Object-Oriented Metrics in Practice. Page 18. */ - private static final int FEW_THRESHOLD = 5; + private static final int FEW_ATFD_THRESHOLD = 6; /** * One third is a low value. See: Lanza. Object-Oriented Metrics in Practice. Page 17. @@ -47,124 +37,21 @@ public class GodClassRule extends AbstractJavaRule { private static final double TCC_THRESHOLD = 1.0 / 3.0; - /** The Access To Foreign Data metric. */ - private int atfdCounter; - - - /** - * Base entry point for the visitor - the compilation unit (everything within one file). The metrics are - * initialized. Then the other nodes are visited. Afterwards the metrics are evaluated against fixed thresholds. - */ @Override public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { int wmc = (int) JavaMetrics.get(JavaClassMetricKey.WMC, node); double tcc = JavaMetrics.get(JavaClassMetricKey.TCC, node); + int atfd = (int) JavaMetrics.get(JavaClassMetricKey.ATFD, node); + super.visit(node, data); - atfdCounter = 0; + if (wmc >= WMC_VERY_HIGH && atfd > FEW_ATFD_THRESHOLD && tcc < TCC_THRESHOLD) { - Object result = super.visit(node, data); - - - // StringBuilder debug = new StringBuilder(); - // debug.append("Values for class ") - // .append(node.getImage()).append(": ") - // .append("WMC=").append(wmcCounter).append(", ") - // .append("ATFD=").append(atfdCounter).append(", ") - // .append("TCC=").append(tcc); - // System.out.println(debug.toString()); - - if (wmc >= WMC_VERY_HIGH && atfdCounter > FEW_THRESHOLD && tcc < TCC_THRESHOLD) { - - String sb = getMessage() + " (" + "WMC=" + wmc + ", " + "ATFD=" - + atfdCounter + ", " + "TCC=" + tcc + ')'; - - RuleContext ctx = (RuleContext) data; - ctx.getReport().addRuleViolation(new JavaRuleViolation(this, ctx, node, sb)); + addViolation(data, node, new Object[] {wmc, + StringUtil.percentageString(tcc, 3, true), + atfd, }); } - return result; + return data; } - - @Override - public Object visit(ASTPrimaryExpression node, Object data) { - if (isForeignAttributeOrMethod(node)) { - if (isAttributeAccess(node) || isMethodCall(node) && isForeignGetterSetterCall(node)) { - atfdCounter++; - } - } - return super.visit(node, data); - } - - - private boolean isForeignGetterSetterCall(ASTPrimaryExpression node) { - - String methodOrAttributeName = getMethodOrAttributeName(node); - - return methodOrAttributeName != null && StringUtil.startsWithAny(methodOrAttributeName, "get", "is", "set"); - } - - - private boolean isMethodCall(ASTPrimaryExpression node) { - boolean result = false; - List suffixes = node.findDescendantsOfType(ASTPrimarySuffix.class); - if (suffixes.size() == 1) { - result = suffixes.get(0).isArguments(); - } - return result; - } - - - private boolean isForeignAttributeOrMethod(ASTPrimaryExpression node) { - boolean result = false; - String nameImage = getNameImage(node); - - if (nameImage != null && (!nameImage.contains(".") || nameImage.startsWith("this."))) { - result = false; - } else if (nameImage == null && node.getFirstDescendantOfType(ASTPrimaryPrefix.class).usesThisModifier()) { - result = false; - } else if (nameImage == null && node.hasDecendantOfAnyType(ASTLiteral.class, ASTAllocationExpression.class)) { - result = false; - } else { - result = true; - } - - return result; - } - - - private String getNameImage(ASTPrimaryExpression node) { - ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); - ASTName name = prefix.getFirstDescendantOfType(ASTName.class); - - String image = null; - if (name != null) { - image = name.getImage(); - } - return image; - } - - - private String getMethodOrAttributeName(ASTPrimaryExpression node) { - ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); - ASTName name = prefix.getFirstDescendantOfType(ASTName.class); - - String methodOrAttributeName = null; - - if (name != null) { - int dotIndex = name.getImage().indexOf("."); - if (dotIndex > -1) { - methodOrAttributeName = name.getImage().substring(dotIndex + 1); - } - } - - return methodOrAttributeName; - } - - - private boolean isAttributeAccess(ASTPrimaryExpression node) { - return node.findDescendantsOfType(ASTPrimarySuffix.class).isEmpty(); - } - - } diff --git a/pmd-java/src/main/resources/rulesets/java/design.xml b/pmd-java/src/main/resources/rulesets/java/design.xml index 49d09261c7..5e63e1e6fe 100644 --- a/pmd-java/src/main/resources/rulesets/java/design.xml +++ b/pmd-java/src/main/resources/rulesets/java/design.xml @@ -1856,7 +1856,7 @@ public class HelloWorldBean { From d85f4ce3413cfed034e8a105fab70826c96bb746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 22 Aug 2017 16:28:36 +0200 Subject: [PATCH 06/11] Documentation --- .../pages/pmd/languages/java_metrics_index.md | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/docs/pages/pmd/languages/java_metrics_index.md b/docs/pages/pmd/languages/java_metrics_index.md index 84fdf4a809..40676da104 100644 --- a/docs/pages/pmd/languages/java_metrics_index.md +++ b/docs/pages/pmd/languages/java_metrics_index.md @@ -11,10 +11,18 @@ toc: ## Access to Foreign Data (ATFD) -*Operation metric, class metric.* +*Operation metric, class metric.* Can be computed on classes, enums and +concrete operations. ### Description +Number of usages of foreign attributes, both directly and through accessors. +High values of ATFD (> 3 for an operation) may suggest that the class or operation +breaks encapsulation by relying on the internal representation of the classes +it uses instead of the services they provide. + +ATFD can be used to detect God Classes and Feature Envy. \[[Lanza05](#Lanza05)\] + ## Cyclomatic Complexity (CYCLO) @@ -226,11 +234,30 @@ between blocks 4 and 5 before jumping to block 6. The first `if` offers 2 choices, the second offers 3, so the cyclomatic complexity of this method is 2 + 3 = 5. NPath, however, sees 2 * 3 = 6 full paths from the beginning to the end. + ## Number Of Public Attributes (NOPA) *Class metric.* Can be computed on classes. ## Number Of Accessor Methods (NOAM) *Class metric.* Can be computed on classes. + +## Tight Class Cohesion (TCC) + +*Class metric.* Can be computed on classes and enums. + +### Description + +The relative number of method pairs of a class that access in common at +least one attribute of the measured class. TCC only counts +direct attribute accesses, that is, only those attributes that are accessed in +the body of the method \[[BK95](#BK95)\]. + +TCC is taken to be a reliable cohesion metric for a class. High values (>70%) +indicate a class with one basic function, which is hard to break into subcomponents. +On the other hand, low values (<50%) may indicate that the class tries to do too much and +defines several unrelated services, which is undesirable. + +TCC can be used to detect God Classes and Brain Classes \[[Lanza05](#Lanza05)\]. ## Weighted Method Count (WMC) @@ -268,6 +295,10 @@ This metric is used to detect Data Classes, in conjunction with [WMC](#weighted- # References + +BK95: Bieman, Kang; Cohesion and reuse in an object-oriented system. +In Proceedings ACM Symposium on Software Reusability, 1995. + Lanza05: Lanza, Marinescu; Object-Oriented Metrics in Practice, 2005. McCabe76: McCabe, A Complexity Measure, in Proceedings of the 2nd ICSE (1976). From 9f1e520ee935c5e95e7bba7d9f1c8ca723e2452d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Thu, 24 Aug 2017 09:09:39 +0200 Subject: [PATCH 07/11] Data class rule displays metric values --- .../pmd/lang/java/metrics/impl/TccMetric.java | 4 +--- .../pmd/lang/java/metrics/rule/DataClassRule.java | 10 +++++++++- pmd-java/src/main/resources/rulesets/java/metrics.xml | 2 +- .../pmd/lang/java/rule/metrics/xml/DataClass.xml | 4 ++-- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java index e5f40398c9..feb05e7b15 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java @@ -31,9 +31,7 @@ public class TccMetric extends AbstractJavaClassMetric { int numPairs = numMethodsRelatedByAttributeAccess(usagesByMethod); int maxPairs = maxMethodPairs(usagesByMethod.size()); - double tcc = maxPairs == 0 ? 0. : numPairs / (double) maxPairs; - - return tcc; + return numPairs / (double) maxPairs; } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java index 777b9df2d6..2e6d405d46 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java @@ -8,6 +8,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; import net.sourceforge.pmd.lang.java.metrics.JavaMetrics; import net.sourceforge.pmd.lang.java.metrics.api.JavaClassMetricKey; import net.sourceforge.pmd.lang.java.rule.AbstractJavaMetricsRule; +import net.sourceforge.pmd.util.StringUtil; /** * @author Clément Fournier @@ -29,7 +30,14 @@ public class DataClassRule extends AbstractJavaMetricsRule { boolean isDataClass = interfaceRevealsData(node) && classRevealsDataAndLacksComplexity(node); if (isDataClass) { - addViolation(data, node, new String[] {node.getImage()}); + double woc = JavaMetrics.get(JavaClassMetricKey.WOC, node); + int nopa = (int) JavaMetrics.get(JavaClassMetricKey.NOPA, node); + int noam = (int) JavaMetrics.get(JavaClassMetricKey.NOAM, node); + int wmc = (int) JavaMetrics.get(JavaClassMetricKey.WMC, node); + + addViolation(data, node, new Object[] {node.getImage(), + StringUtil.percentageString(woc, 3, true), + nopa, noam, wmc}); } return super.visit(node, data); diff --git a/pmd-java/src/main/resources/rulesets/java/metrics.xml b/pmd-java/src/main/resources/rulesets/java/metrics.xml index f6eaebfe4c..2da230c93b 100644 --- a/pmd-java/src/main/resources/rulesets/java/metrics.xml +++ b/pmd-java/src/main/resources/rulesets/java/metrics.xml @@ -165,7 +165,7 @@ public class Foo { diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/metrics/xml/DataClass.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/metrics/xml/DataClass.xml index 3a73eb3c0f..793ead74bf 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/metrics/xml/DataClass.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/metrics/xml/DataClass.xml @@ -59,7 +59,7 @@ public class Property implements Comparable { // Woc = 10%, Noam = 6, Nopa = 0, ArgoUML data class 1 - The class 'Property' is suspected to be a Data Class + The class 'Property' is suspected to be a Data Class (WOC=11.111%, NOPA=0, NOAM=6, WMC=9) @@ -206,7 +206,7 @@ public class TestDescriptor { PMD data class 1 - The class 'TestDescriptor' is suspected to be a Data Class + The class 'TestDescriptor' is suspected to be a Data Class (WOC=13.043%, NOPA=0, NOAM=17, WMC=25) From f631abc6d3de0fd7ac149bea98cd2f8dd02acd84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Thu, 24 Aug 2017 09:53:30 +0200 Subject: [PATCH 08/11] Fix tests --- .../lang/java/metrics/impl/AtfdMetric.java | 7 +++++ .../lang/java/metrics/rule/DataClassRule.java | 2 +- .../lang/java/metrics/impl/xml/AtfdTest.xml | 28 ++++++++++--------- .../lang/java/metrics/impl/xml/TccTest.xml | 5 +--- 4 files changed, 24 insertions(+), 18 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java index 946c13ef78..3761ba53f8 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/AtfdMetric.java @@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.java.metrics.impl; import org.apache.commons.lang3.mutable.MutableInt; import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration; import net.sourceforge.pmd.lang.java.metrics.JavaMetrics; import net.sourceforge.pmd.lang.java.metrics.api.JavaOperationMetricKey; @@ -24,6 +25,12 @@ public final class AtfdMetric { public static final class AtfdOperationMetric extends AbstractJavaOperationMetric { + @Override + public boolean supports(ASTMethodOrConstructorDeclaration node) { + return node instanceof ASTMethodDeclaration && super.supports(node); + } + + @Override public double computeFor(ASTMethodOrConstructorDeclaration node, MetricOptions options) { return ((MutableInt) node.jjtAccept(new AtfdBaseVisitor(), new MutableInt(0))).getValue(); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java index 2e6d405d46..4049282b8c 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java @@ -37,7 +37,7 @@ public class DataClassRule extends AbstractJavaMetricsRule { addViolation(data, node, new Object[] {node.getImage(), StringUtil.percentageString(woc, 3, true), - nopa, noam, wmc}); + nopa, noam, wmc, }); } return super.visit(node, data); diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml index 6a060ba0ce..ef94be4d92 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml @@ -123,25 +123,32 @@ public class StatementAndBraceFinder extends JavaParserVisitorAdapter { - Full example - 1 + Full example + 7 - '.StatementAndBraceFinder' has value 10. + '.StatementAndBraceFinder' has value 10 highest 6. + '.StatementAndBraceFinder#buildDataFlowFor(JavaNode)' has value 6. + '.StatementAndBraceFinder#tryToLog(String, NodeType, Node)' has value 4. + '.StatementAndBraceFinder#tryToLog(NodeType, Node)' has value 0. + '.StatementAndBraceFinder#visit(ASTStatementExpression, Object)' has value 4. + '.StatementAndBraceFinder#visit(ASTVariableDeclarator, Object)' has value 4. + '.StatementAndBraceFinder#visit(ASTExpression, Object)' has value 18. - Test empty class + TODO: known limitation, should report 1 1 false - '.Foo' has value 0. + '.Foo#bar()' has value 0. @@ -152,7 +159,7 @@ public class StatementAndBraceFinder extends JavaParserVisitorAdapter { 1 - '.Foo' has value 0. + '.Foo' has value 0 highest 0. - NOPA doesn't support enums, interfaces or annotations + ATFD doesn't support interfaces or annotations 0 Test empty class - 1 - - '.Foo' has value 0. - + 0 Date: Thu, 24 Aug 2017 23:55:13 +0200 Subject: [PATCH 09/11] Corrections for PR #578 --- .../net/sourceforge/pmd/util/StringUtil.java | 28 ++----------------- .../pmd/lang/java/metrics/impl/TccMetric.java | 2 +- .../lang/java/metrics/rule/DataClassRule.java | 2 +- .../lang/java/rule/design/GodClassRule.java | 4 +-- .../metrics/impl/AbstractMetricTestRule.java | 11 ++++---- .../lang/java/metrics/impl/xml/AtfdTest.xml | 4 +-- .../lang/java/metrics/impl/xml/TccTest.xml | 2 +- .../lang/java/metrics/impl/xml/WocTest.xml | 2 +- 8 files changed, 16 insertions(+), 39 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java index a4cd8772e7..57bc4666bc 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java @@ -8,6 +8,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; import java.util.List; +import java.util.Locale; import org.apache.commons.lang3.StringUtils; @@ -29,43 +30,20 @@ public final class StringUtil { * * @param val a double value between 0 and 1 * @param numDecimals The number of decimal places to keep - * @param truncateInt Whether to truncate the string to an int if possible (if this is set, then 10.0% will be - * represented as "10%") * * @return A formatted string * * @throws IllegalArgumentException if the double to format is not between 0 and 1 */ - public static String percentageString(double val, int numDecimals, boolean truncateInt) { + public static String percentageString(double val, int numDecimals) { if (val < 0 || val > 1) { throw new IllegalArgumentException("Expected a number between 0 and 1"); } - return truncateDouble(100 * val, numDecimals, truncateInt) + "%"; + return String.format(Locale.ROOT, "%." + numDecimals + "f%%", 100 * val); } - /** - * Returns a string representation of the double {@code val}, truncated to {@code numDecimal} decimal places. - * - * @param val The value to present - * @param numDecimals The number of decimal places to keep - * @param truncateInt Whether to truncate the string to an int if possible (if this is set, then 10.0 will be - * represented as "10") - * - * @return A string representation of the double number - */ - public static String truncateDouble(double val, int numDecimals, boolean truncateInt) { - int factor = (int) Math.pow(10, numDecimals); - double truncated = Math.floor(factor * val) / factor; - - if (truncateInt && truncated == Math.floor(truncated)) { - return String.valueOf((int) truncated); - } else { - return String.valueOf(truncated); - } - } - /** * Return whether the non-null text arg starts with any of the prefix * values. diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java index feb05e7b15..5823e27b56 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/TccMetric.java @@ -48,7 +48,7 @@ public class TccMetric extends AbstractJavaClassMetric { int pairs = 0; if (methodCount > 1) { - for (int i = 0; i < methodCount; i++) { + for (int i = 0; i < methodCount - 1; i++) { for (int j = i + 1; j < methodCount; j++) { String firstMethodName = methods.get(i); String secondMethodName = methods.get(j); diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java index 4049282b8c..a8aaf78146 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/rule/DataClassRule.java @@ -36,7 +36,7 @@ public class DataClassRule extends AbstractJavaMetricsRule { int wmc = (int) JavaMetrics.get(JavaClassMetricKey.WMC, node); addViolation(data, node, new Object[] {node.getImage(), - StringUtil.percentageString(woc, 3, true), + StringUtil.percentageString(woc, 3), nopa, noam, wmc, }); } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java index a10b481a74..8e610ee758 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/GodClassRule.java @@ -31,7 +31,7 @@ public class GodClassRule extends AbstractJavaRule { /** * Few means between 2 and 5. See: Lanza. Object-Oriented Metrics in Practice. Page 18. */ - private static final int FEW_ATFD_THRESHOLD = 6; + private static final int FEW_ATFD_THRESHOLD = 5; /** * One third is a low value. See: Lanza. Object-Oriented Metrics in Practice. Page 17. @@ -50,7 +50,7 @@ public class GodClassRule extends AbstractJavaRule { if (wmc >= WMC_VERY_HIGH && atfd > FEW_ATFD_THRESHOLD && tcc < TCC_THRESHOLD) { addViolation(data, node, new Object[] {wmc, - StringUtil.percentageString(tcc, 3, true), + StringUtil.percentageString(tcc, 3), atfd, }); } return data; diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java index 909e8767fd..ebe495ba28 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java @@ -6,6 +6,7 @@ package net.sourceforge.pmd.lang.java.metrics.impl; import java.util.Collections; import java.util.HashMap; +import java.util.Locale; import java.util.Map; import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration; @@ -128,13 +129,11 @@ public abstract class AbstractMetricTestRule extends AbstractJavaMetricsRule { /** Gets a nice string representation of a double. */ - public String niceDoubleString(double val) { - int numDecimalPlaces = 2; - - if (val > 0 && val < 1) { // percentage - return StringUtil.percentageString(val, numDecimalPlaces, true); + private String niceDoubleString(double val) { + if (val == (int) val) { + return String.valueOf((int) val); } else { - return String.valueOf(StringUtil.truncateDouble(val, numDecimalPlaces, true)); + return String.format(Locale.ROOT, "%." + 4 + "f", val); } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml index ef94be4d92..56c4f928d9 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/AtfdTest.xml @@ -137,12 +137,12 @@ public class StatementAndBraceFinder extends JavaParserVisitorAdapter { - + TODO: known limitation, should report 1 1 false - '.Foo#bar()' has value 0. + '.Foo#bar()' has value 1. Full example 1 - '.Property' has value 9.52%. + '.Property' has value 0.0952. diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/WocTest.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/WocTest.xml index 303509b4ed..469db46859 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/WocTest.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/WocTest.xml @@ -66,7 +66,7 @@ return _name.compareTo(((Property) o)._name); Full example 1 - '.Property' has value 11.11%. + '.Property' has value 0.1111. From 5a2fc3e2c3b34e172c3a9f82fc8dadfdbe4f6441 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Fri, 25 Aug 2017 00:20:45 +0200 Subject: [PATCH 10/11] Checkstyle --- .../pmd/lang/java/metrics/impl/AbstractMetricTestRule.java | 1 - 1 file changed, 1 deletion(-) diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java index ebe495ba28..87943298c4 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/metrics/impl/AbstractMetricTestRule.java @@ -22,7 +22,6 @@ import net.sourceforge.pmd.lang.metrics.ResultOption; import net.sourceforge.pmd.lang.rule.properties.BooleanProperty; import net.sourceforge.pmd.lang.rule.properties.DoubleProperty; import net.sourceforge.pmd.lang.rule.properties.EnumeratedMultiProperty; -import net.sourceforge.pmd.util.StringUtil; /** * Abstract test rule for a metric. Tests of metrics use the standard framework for rule testing, using one dummy rule From f6be3b86206dd830cf69d59f83373ab26e72ce94 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 26 Aug 2017 10:56:20 +0200 Subject: [PATCH 11/11] Update release notes, refs #578 --- docs/pages/release_notes.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 200439eb31..02fbe9e204 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -89,6 +89,8 @@ and include them to such reports. * The rule `UncommentedEmptyConstructor` (ruleset `java-design`) will now ignore empty constructors annotated with `javax.inject.Inject`. +* The rule `GodClass` (ruleset `java-design`) has been revamped to use the new metrics framework. + #### Removed Rules * The deprecated rule `UseSingleton` has been removed from the ruleset `java-design`. The rule has been renamed @@ -233,4 +235,5 @@ All existing rules have been updated to reflect these changes. If you have custo * [#563](https://github.com/pmd/pmd/pull/563): \[java] Add support for basic method type inference for strict invocation - [Bendegúz Nagy](https://github.com/WinterGrascph) * [#567](https://github.com/pmd/pmd/pull/567): \[java] Last API change for metrics (metric options) - [Clément Fournier](https://github.com/oowekyala) * [#573](https://github.com/pmd/pmd/pull/573): \[java] Data class rule - [Clément Fournier](https://github.com/oowekyala) -* [#576](https://github.com/pmd/pmd/pull/576): \[doc][java] Add hint for Guava users in InefficientEmptyStringCheck - [mmoehring](https://github.com/mmoehring) +* [#576](https://github.com/pmd/pmd/pull/576): \[doc]\[java] Add hint for Guava users in InefficientEmptyStringCheck - [mmoehring](https://github.com/mmoehring) +* [#578](https://github.com/pmd/pmd/pull/578): \[java] Refactored god class rule - [Clément Fournier](https://github.com/oowekyala)