Merge branch 'master' of https://github.com/kshantaramanUFL/pmd into kshantaramanUFL-master

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingleMethodSingletonRule.java

@@ -0,0 +1,78 @@
+package net.sourceforge.pmd.lang.java.rule.design;
+
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBodyDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
+import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
+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.ASTReturnStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+
+public class SingleMethodSingletonRule extends AbstractJavaRule {
+
+	private static Map<String, ASTFieldDeclaration> fieldDecls = new HashMap<String, ASTFieldDeclaration>();
+	private static Set<ASTFieldDeclaration> returnset = new HashSet<ASTFieldDeclaration>();
+	boolean violation=false;
+	private static Set<String> methodset = new HashSet<String>();
+	@Override
+	public Object visit(ASTFieldDeclaration node, Object data) {
+		
+		if (node.isStatic() && node.isPrivate()) {
+				ASTVariableDeclaratorId varDeclaratorId=node.getFirstDescendantOfType(ASTVariableDeclaratorId.class);
+				if(varDeclaratorId!=null){
+					String varName=varDeclaratorId.getImage();
+					fieldDecls.put(varName, node);
+				}
+			}
+		
+		return super.visit(node, data);
+	}
+
+	@Override
+	public Object visit(ASTCompilationUnit node, Object data){
+		violation=false;
+		fieldDecls.clear();
+		returnset.clear();
+		methodset.clear();
+		return super.visit(node,data);
+	}
+	@Override
+	public Object visit(ASTMethodDeclaration node, Object data) {
+
+		violation=false;
+		if (node.getResultType().isVoid()) {
+			return super.visit(node, data);
+		}
+
+		if ("getInstance".equals(node.getMethodName())) {
+
+			if(!methodset.add(node.getMethodName())){
+				violation=true;
+			}
+		}
+		
+		if(violation){
+			addViolation(data, node);
+		}
+		return super.visit(node, data);
+	}
+
+	private String getNameFromPrimaryPrefix(ASTPrimaryPrefix pp) {
+		if ((pp.jjtGetNumChildren() == 1)
+				&& (pp.jjtGetChild(0) instanceof ASTName)) {
+			return ((ASTName) pp.jjtGetChild(0)).getImage();
+		}
+		return null;
+	}
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingletonClassReturningNewInstanceRule.java

@@ -0,0 +1,125 @@
+package net.sourceforge.pmd.lang.java.rule.design;
+
+import java.util.List;
+
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
+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.ASTReturnStatement;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+
+public class SingletonClassReturningNewInstanceRule extends AbstractJavaRule {
+	
+	@Override
+	public Object visit(ASTMethodDeclaration node, Object data) {
+
+		boolean violation = false;
+		String localVarName = null;
+		String returnVariableName = null;
+
+		if (node.getResultType().isVoid()) {
+			return super.visit(node, data);
+		}
+
+		if ("getInstance".equals(node.getMethodName())) {
+			List<ASTReturnStatement> rsl = node
+					.findDescendantsOfType(ASTReturnStatement.class);
+			if (rsl.isEmpty()) {
+				return super.visit(node, data);
+			} else {
+				for(ASTReturnStatement rs : rsl){
+				
+				List<ASTPrimaryExpression> pel = rs
+						.findDescendantsOfType(ASTPrimaryExpression.class);
+				ASTPrimaryExpression ape = pel.get(0);
+				if (ape.getFirstDescendantOfType(ASTAllocationExpression.class) != null) {
+					violation = true;
+					break;
+					}
+				}
+			}
+
+			/*
+			 * public class Singleton {
+			 * 
+			 * private static Singleton m_instance=null;
+			 * 
+			 * public static Singleton getInstance() {
+			 * 
+			 * Singleton m_instance=null;
+			 * 
+			 * if ( m_instance == null ) { 
+			 * 		synchronized(Singleton.class) { 
+			 * 			if(m_instance == null) { 
+			 * 						m_instance = new Singleton(); 
+			 * 			} 
+			 * 		} 
+			 * } 
+			 * return m_instance; 
+			 * } 
+			 * }
+			 * 
+			 */
+
+			List<ASTBlockStatement> ASTBlockStatements = node
+					.findDescendantsOfType(ASTBlockStatement.class);
+			returnVariableName = getReturnVariableName(node);
+			if (ASTBlockStatements.size() != 0) {
+				for (ASTBlockStatement blockStatement : ASTBlockStatements) {
+					if (blockStatement
+							.hasDescendantOfType(ASTLocalVariableDeclaration.class)) {
+						List<ASTLocalVariableDeclaration> lVarList=blockStatement.findDescendantsOfType(ASTLocalVariableDeclaration.class);
+						if(!lVarList.isEmpty()){
+							for(ASTLocalVariableDeclaration localVar : lVarList){
+								localVarName = localVar.getVariableName();
+								if (returnVariableName != null
+										&& returnVariableName.equals(localVarName)) {
+									violation = true;
+									break;
+								}
+							}
+						}
+					}
+				}
+			}
+		}
+		if (violation) {
+			addViolation(data, node);
+		}
+		return super.visit(node, data);
+	}
+
+	private String getReturnVariableName(ASTMethodDeclaration node) {
+
+		List<ASTReturnStatement> rsl = node
+				.findDescendantsOfType(ASTReturnStatement.class);
+		ASTReturnStatement rs = rsl.get(0);
+		List<ASTPrimaryExpression> pel = rs
+				.findDescendantsOfType(ASTPrimaryExpression.class);
+		ASTPrimaryExpression ape = pel.get(0);
+		Node lastChild = ape.jjtGetChild(0);
+		String returnVariableName = null;
+		if (lastChild instanceof ASTPrimaryPrefix) {
+			returnVariableName = getNameFromPrimaryPrefix((ASTPrimaryPrefix) lastChild);
+		}
+		/*
+		 * if(lastChild instanceof ASTPrimarySuffix){ returnVariableName =
+		 * getNameFromPrimarySuffix((ASTPrimarySuffix) lastChild); }
+		 */
+		return returnVariableName;
+
+	}
+
+	private String getNameFromPrimaryPrefix(ASTPrimaryPrefix pp) {
+		if ((pp.jjtGetNumChildren() == 1)
+				&& (pp.jjtGetChild(0) instanceof ASTName)) {
+			return ((ASTName) pp.jjtGetChild(0)).getImage();
+		}
+		return null;
+	}
+}

pmd-java/src/main/resources/rulesets/java/design.xml

@@ -1297,6 +1297,59 @@ public static Foo getFoo() {
         ]]></example>
     </rule>
 
+<rule name="SingleMethodSingleton"
+		since="5.4"
+		externalInfoUrl=""
+		message="Class contains multiple getInstance methods. Please review."
+		class="net.sourceforge.pmd.lang.java.rule.design.SingleMethodSingletonRule">
+<description>
+Some classes contain overloaded getInstance. The problem with overloaded getInstance methods
+is that the instance created using the overloaded method is not cached and so, 
+ for each call and new objects will be created for every invocation.
+</description>
+<priority>2</priority>
+<example><![CDATA[
+public class Singleton {
+
+   private static Singleton singleton = new Singleton( );
+   
+  private Singleton(){ }
+   
+public static Singleton getInstance( ) {
+	  return singleton;
+}
+public static Singleton getInstance(Object obj){
+	Singleton singleton = (Singleton) obj;
+	return singleton;			//violation
+}
+}
+]]>
+</example>
+</rule>
+
+<rule name="SingletonClassReturningNewInstance"
+		since="5.4"
+		externalInfoUrl=""
+		message="getInstance method always creates a new object and hence does not comply to Singleton Design Pattern behaviour. Please review"
+		class="net.sourceforge.pmd.lang.java.rule.design.SingletonClassReturningNewInstanceRule">
+<description>
+Some classes contain overloaded getInstance. The problem with overloaded getInstance methods
+is that the instance created using the overloaded method is not cached and so, 
+ for each call and new objects will be created for every invocation.
+</description>
+<priority>2</priority>
+<example><![CDATA[
+class Singleton {
+	private static Singleton instance = null;
+	public static Singleton getInstance() {
+	synchronized(Singleton.class){
+		return new Singleton();					
+	}
+	}
+}
+]]>
+</example>
+</rule>
 
     <rule deprecated="true" name="UncommentedEmptyMethod" ref="UncommentedEmptyMethodBody" />
     <rule name="UncommentedEmptyMethodBody"

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/DesignRulesTest.java

@@ -46,6 +46,8 @@ public class DesignRulesTest extends SimpleAggregatorTst {
         addRule(RULESET, "NonCaseLabelInSwitchStatement");
         addRule(RULESET, "NonStaticInitializer");
         addRule(RULESET, "NonThreadSafeSingleton");
+        addRule(RULESET,"SingleMethodSingleton");
+        addRule(RULESET, "SingletonClassReturningNewInstance");
         addRule(RULESET, "OptimizableToArrayCall");
         // addRule(RULESET, "PositionalIteratorRule"); This rule does not yet
         // exist

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/SingleMethodSingleton.xml

@@ -0,0 +1,48 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data>
+	<test-code>
+		<description><![CDATA[
+Not OK! Has overriden getInstance() methods
+     ]]></description>
+		<expected-problems>1</expected-problems>
+		<code>
+        <![CDATA[
+public class Singleton {
+
+   private static Singleton singleton = new Singleton();
+   
+  private Singleton(){ }
+   
+public static Singleton getInstance() {
+      return singleton;
+}
+public static Singleton getInstance(Object obj){
+	Singleton singleton = (Singleton) obj;
+	return singleton;
+}
+}
+]]>
+		</code>
+	</test-code>
+	<test-code>
+		<description><![CDATA[
+OK! Has only one Singleton
+     ]]></description>
+		<expected-problems>0</expected-problems>
+		<code>
+        <![CDATA[
+public class Singleton{
+private static Singleton instance = null;
+    public static Singleton getInstance() {
+	synchronized(Singleton.class){
+		if(instance==null){
+    	instance = new Singleton();
+    	}
+	}
+    	return instance;
+    }
+}
+]]>
+		</code>
+	</test-code>
+</test-data>

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/SingletonClassReturningNewInstance.xml

@@ -0,0 +1,64 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data>
+	<test-code>
+		<description><![CDATA[
+failure case
+     ]]></description>
+		<expected-problems>1</expected-problems>
+		<code>
+        <![CDATA[
+public class Singleton {
+    private static Singleton instance = null;
+    public static Singleton getInstance() {
+	synchronized(Singleton.class){
+    	return new Singleton();
+	}
+    }
+}
+]]>
+		</code>
+	</test-code>
+	<test-code>
+		<description><![CDATA[
+failure case
+     ]]></description>
+		<expected-problems>1</expected-problems>
+		<code>
+        <![CDATA[
+public class Singleton{
+private static Singleton instance = null;
+    public static Singleton getInstance() {
+	synchronized(Singleton.class){
+		if(instance==null){
+    	Singleton instance = new Instance();
+    	return instance;
+    	}
+	}
+	return instance;
+    }
+}
+]]>
+		</code>
+	</test-code>
+	<test-code>
+		<description><![CDATA[
+Works! Does not return locally created variable     
+]]></description>
+		<expected-problems>0</expected-problems>
+		<code>
+        <![CDATA[
+public class Singleton{
+private static Singleton instance = null;
+    public static Singleton getInstance() {
+	synchronized(Singleton.class){
+		if(instance==null){
+    	instance = new Instance();
+    	}
+	}
+    	return instance;
+    }
+}
+]]>
+		</code>
+	</test-code>
+</test-data>