diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/ClassStats.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/ClassStats.java index 0a884d5270..40ff59d50c 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/ClassStats.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/ClassStats.java @@ -128,8 +128,7 @@ import net.sourceforge.pmd.lang.java.oom.signature.OperationSignature; /** - * Checks whether the class declares an operation by the name given which is covered by the - * signature mask. + * Checks whether the class declares an operation by the name given which is covered by the signature mask. * * @param name The name of the operation to look for * @param mask The mask covering accepted signatures @@ -151,8 +150,7 @@ import net.sourceforge.pmd.lang.java.oom.signature.OperationSignature; /** - * Checks whether the class declares a field by the name given which is covered by the - * signature mask. + * Checks whether the class declares a field by the name given which is covered by the signature mask. * * @param name The name of the field to look for * @param mask The mask covering accepted signatures @@ -226,7 +224,7 @@ import net.sourceforge.pmd.lang.java.oom.signature.OperationSignature; /* default */ double compute(OperationMetricKey key, ASTMethodOrConstructorDeclaration node, String name, boolean force, MetricVersion version) { - // TODO:cf the operation signature might be built many times, consider profiling + // TODO:cf the operation signature might be built many times, consider storing it in the node Map sigMap = operations.get(OperationSignature.buildFor(node)); if (sigMap == null) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/Metrics.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/Metrics.java index 42f55f082c..9dfe2c8194 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/Metrics.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/Metrics.java @@ -33,8 +33,7 @@ public final class Metrics { * * @return The top level package stats */ - /* default */ - static PackageStats getTopLevelPackageStats() { + /* default */ static PackageStats getTopLevelPackageStats() { return TOP_LEVEL_PACKAGE; } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/signature/OperationSignature.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/signature/OperationSignature.java index 5004bbaecb..ec9928c742 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/signature/OperationSignature.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/signature/OperationSignature.java @@ -5,24 +5,16 @@ package net.sourceforge.pmd.lang.java.oom.signature; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.regex.Pattern; import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters; -import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration; -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.ASTReturnStatement; -import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; +import net.sourceforge.pmd.lang.java.ast.ASTResultType; import net.sourceforge.pmd.lang.java.ast.ASTType; import net.sourceforge.pmd.lang.java.symboltable.ClassScope; import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; @@ -35,8 +27,8 @@ import net.sourceforge.pmd.lang.symboltable.NameOccurrence; */ public final class OperationSignature extends Signature { - private static final Map POOL = new HashMap<>(); + private static final Map POOL = new HashMap<>(); public final Role role; public final boolean isAbstract; @@ -47,6 +39,19 @@ public final class OperationSignature extends Signature { this.isAbstract = isAbstract; } + + @Override + public boolean equals(Object o) { + return this == o; + } + + + @Override + public int hashCode() { + return (isAbstract ? 1 : 0) + super.hashCode() << 1 + role.hashCode() << 2; + } + + /** * Builds an operation signature from a method or constructor declaration. * @@ -62,20 +67,12 @@ public final class OperationSignature extends Signature { return POOL.get(code); } + /** Used internally by the pooler. */ private static int code(Visibility visibility, Role role, boolean isAbstract) { return visibility.hashCode() * 31 + role.hashCode() * 2 + (isAbstract ? 1 : 0); } - @Override - public boolean equals(Object o) { - return this == o; - } - - @Override - public int hashCode() { - return (isAbstract ? 1 : 0) + super.hashCode() << 1 + role.hashCode() << 2; - } /** * Role of an operation. @@ -83,7 +80,8 @@ public final class OperationSignature extends Signature { public enum Role { GETTER_OR_SETTER, CONSTRUCTOR, METHOD, STATIC; - private static final Pattern NAME_PATTERN = Pattern.compile("(?:get|set|is|increment|decrement)\\w*"); + + private static final Pattern GETTER_OR_SETTER_NAME_PATTERN = Pattern.compile("(?:get|set|is)\\w*"); public static Role get(ASTMethodOrConstructorDeclaration node) { @@ -101,25 +99,12 @@ public final class OperationSignature extends Signature { } } - // TODO:cf clean that up private static boolean isGetterOrSetter(ASTMethodDeclaration node) { String name = node.getName(); - if (NAME_PATTERN.matcher(name).matches()) { + if (GETTER_OR_SETTER_NAME_PATTERN.matcher(name).matches()) { return true; } - if (node.isAbstract()) { - return false; - } - - int length = node.getEndLine() - node.getBeginLine(); - - if (length > 6) { - return false; - } else if (length > 4 && node.getFirstDescendantOfType(ASTIfStatement.class) == null) { - return false; - } - ClassScope scope = node.getScope().getEnclosingScope(ClassScope.class); // fields names mapped to their types @@ -137,62 +122,27 @@ public final class OperationSignature extends Signature { return isGetter(node, fieldNames) || isSetter(node, fieldNames); } + /** Attempts to determine if the method is a getter. */ private static boolean isGetter(ASTMethodDeclaration node, Map fieldNames) { - - List returnStatements - = node.getBlock().findDescendantsOfType(ASTReturnStatement.class); - - for (ASTReturnStatement st : returnStatements) { - ASTName name = st.getFirstDescendantOfType(ASTName.class); - if (name == null) { - continue; - } - - if (fieldNames.containsKey(name.getImage().split("\\.")[0])) { - return true; - } + if (node.getFirstDescendantOfType(ASTFormalParameters.class).getParameterCount() != 0 + || node.getFirstDescendantOfType(ASTResultType.class).isVoid()) { + return false; } + return fieldNames.containsKey(node.getName()); - return false; } + /** Attempts to determine if the method is a setter. */ private static boolean isSetter(ASTMethodDeclaration node, Map fieldNames) { - if (node.getFirstDescendantOfType(ASTFormalParameters.class).jjtGetNumChildren() != 1) { + if (node.getFirstDescendantOfType(ASTFormalParameters.class).getParameterCount() != 1) { return false; } - List statementExpressions - = node.getBlock().findDescendantsOfType(ASTStatementExpression.class); - Set namesToCheck = new HashSet<>(); - - for (ASTStatementExpression st : statementExpressions) { - ASTName name = st.getFirstDescendantOfType(ASTName.class); - if (name == null) { - // not an assignment, check for method - ASTPrimaryExpression prim = st.getFirstChildOfType(ASTPrimaryExpression.class); - ASTPrimaryPrefix prefix = prim.getFirstChildOfType(ASTPrimaryPrefix.class); - - if (prefix.usesThisModifier() || prefix.usesSuperModifier()) { - namesToCheck.add(prim.getFirstChildOfType(ASTPrimarySuffix.class).getImage()); - } else { - namesToCheck.add(prefix.getImage().split("\\.")[0]); - } - } else { - // this is a direct assignment - namesToCheck.add(name.getImage().split("\\.")[0]); - } - } - - for (String name : namesToCheck) { - if (fieldNames.containsKey(name)) { - return true; - } - } - return false; + return node.getFirstDescendantOfType(ASTResultType.class).isVoid(); } } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/oom/testdata/GetterDetection.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/oom/testdata/GetterDetection.java index ca551bd9ba..705ebce042 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/oom/testdata/GetterDetection.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/oom/testdata/GetterDetection.java @@ -16,19 +16,23 @@ public class GetterDetection { private MutableInt mutX; private boolean bool; + public int getValue() { return value; } + public boolean isBooleanTrue() { return bool; } + public int value() { return value; } - public double speedModified() { + + /* public double speedModified() { return speed * 12 + 1; } @@ -51,5 +55,5 @@ public class GetterDetection { public int mutableIntConditional() { return mutX == null ? 0 : mutX.getValue(); } - +*/ }