Add GuardLogStatement

Based on Heiko Rupp implemetation: http://pilhuhn.blogspot.com/2008/09/pmd-rule-to-check-for-unguarded.html

pmd/src/main/java/net/sourceforge/pmd/RuleSetFactory.java

@@ -321,9 +321,9 @@ public class RuleSetFactory {
 		}
 
 		String attribute = ruleElement.getAttribute("class");
-		Class<?> c = classLoader.loadClass(attribute);
-		Rule rule = (Rule) c.newInstance();
-
+		if ( attribute == null || "".equals(attribute))
+			throw new IllegalArgumentException("The 'class' field of rule can't be null, nor empty.");
+		Rule rule = (Rule) classLoader.loadClass(attribute).newInstance();
 		rule.setName(ruleElement.getAttribute("name"));
 
 		if (ruleElement.hasAttribute("language")) {

pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementRule.java

@@ -0,0 +1,88 @@
+package net.sourceforge.pmd.lang.java.rule.logging;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import net.sourceforge.pmd.Rule;
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
+import net.sourceforge.pmd.lang.java.rule.optimizations.AbstractOptimizationRule;
+
+
+/**
+ * Check that log.debug and log.trace statements are guarded by some
+ * log.isDebugEnabled() or log.isTraceEnabled() checks.
+ * 
+ * @author Heiko hwr@pilhuhn.de
+ * @author Romain Pelisse - <belaran@gmail.com>
+ *
+ */
+public class GuardLogStatementRule extends AbstractOptimizationRule implements Rule {
+
+	private static final Map<String,String> guardStmtByLogLevel = new HashMap<String,String>(5);
+	
+	public GuardLogStatementRule() {
+		guardStmtByLogLevel.put(".trace","isTraceEnabled");
+		guardStmtByLogLevel.put(".debug","isDebugEnabled");
+		guardStmtByLogLevel.put(".warn", "isWarnEnabled");
+		guardStmtByLogLevel.put(".error", "isErrorEnabled");
+		guardStmtByLogLevel.put(".info","isInfoEnabled");
+	}
+	
+	private String lastPrefix(String string) {
+		return (string != null && ! "".equals(string)) ? string.substring(string.lastIndexOf('.'), string.length()) : string;
+	}
+	
+	public Object visit(ASTName name, Object data) {
+			Node node = name.jjtGetParent();
+			if ( node instanceof ASTPrimaryPrefix ) {
+				
+			} else
+				return super.visit(name,data);
+			if (name != null) {
+				String lastPrefix = lastPrefix(name.getImage());
+				if ( guardStmtByLogLevel.keySet().contains(lastPrefix)) {
+					// TODO check for type 
+					Node parent1 = name.getNthParent(5);
+					boolean guardFound = false;
+					if (parent1 instanceof ASTIfStatement) {
+						guardFound = checkForGuard((ASTIfStatement) parent1, lastPrefix);
+					}
+					else if (parent1 instanceof ASTBlockStatement) {
+						Node parent2 = name.getNthParent(7);
+						if (parent2 instanceof ASTIfStatement) {
+							guardFound = checkForGuard((ASTIfStatement) parent2, lastPrefix);						
+						}
+					}
+					if (!guardFound)
+						addViolation(data, name);
+				}
+			} 
+		return super.visit(name, data);		
+	}
+			
+	/**
+	 * We stand on an if() check if it contains log.isDebugEnabled()
+	 * @param stm
+	 * @param isTrace true if log.trace() is used
+	 * @return true if guard was found
+	 */
+	
+	private boolean checkForGuard(ASTIfStatement stm, String logLevel) {
+
+		List<ASTName> names = stm.findDescendantsOfType(ASTName.class);
+		if (names == null || names.isEmpty() )
+			return false;
+
+		for ( ASTName name : names) {
+			String image = name.getImage();
+			if ( guardStmtByLogLevel.get(logLevel).equals(name.getImage()) );
+				return true;
+		}
+		return false;
+	}
+}

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

@@ -63,43 +63,27 @@ public class Foo{
 }
 ]]>
      </example>
-     </rule>
-    
-<!-- This rule will probably be implemented in a java class, since we need to search the logger name 
-first -->
-<!--
-     <rule name="LogBlockWithoutIf"
-         message="There is log block not surrounded by if"
-         class="net.sourceforge.pmd.lang.rule.XPathRule">
-     <description>
-When many log statements are used, it is convenient to surround them with an if statement.
-     </description>
-     <properties>
-         <property name="xpath">
-             <value>
-                 <![CDATA[
-                ]]>
-             </value>
-         </property>
-     </properties>
-        <priority>2</priority>
+   </rule>
 
+   <rule name="GuardLogStatement"
+         language="java"
+         since="5.0"
+         message="There is log block not surrounded by if"
+         class="net.sourceforge.pmd.lang.java.rule.logging.GuardLogStatementRule"
+         externalInfoUrl="${pmd.website.baseurl}/rules/java/logging-java.html#GuardLogStatement">
+     <description>
+Whenever using a log level, one should check if the loglevel is actually enabled, or
+otherwise skip the associate String creation and manipulation.
+     </description>
+     <priority>2</priority>
      <example>
  <![CDATA[
-
  	// Add this for performance
-	// if (log.isLoggable(Level.FINE)) { ...
- 	log.fine("This happens");
-	log.fine("Value of x:"+x);
-	log.fine("Value of y:"+y);
-	log.fine("Value of z:"+z);
-	log.fine("Value of w:"+w);
- 
+	if (log.isLoggable(Level.FINE)) { ...
+ 	    log.fine("This happens");
 ]]>
      </example>
-     </rule>
-    -->
-
+   </rule>
 
     <rule name="SystemPrintln"
    		language="java"
@@ -175,5 +159,5 @@ class Foo {
 ]]>
            </example>
     </rule>
-    
-</ruleset>
+
+</ruleset>

pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjava/LoggingJavaRulesTest.java

@@ -15,6 +15,7 @@ public class LoggingJavaRulesTest extends SimpleAggregatorTst {
         addRule(RULESET, "LoggerIsNotStaticFinal");
         addRule(RULESET, "MoreThanOneLogger");
         addRule(RULESET, "SystemPrintln");
+        addRule(RULESET, "GuardLogStatement");
     }
 
     public static junit.framework.Test suite() {

pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/loggingjava/xml/GuardLogStatement.xml

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data>
+    <test-code>
+        <description><![CDATA[
+OK, guard is here
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+  private static final Logger logger = Logger.getLogger(Foo.class);
+
+  private void foo() {
+    if ( logger.isDebugEnabled() )
+        logger.debug("Debug statement");
+ }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+KO, missing guard
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+  private static final Logger logger = Logger.getLogger(Foo.class);
+
+  private void foo() {
+        logger.debug("Debug statement");
+  }
+}
+     ]]></code>
+    </test-code>
+</test-data>