Merge pull request #93 from cflint/complexBooleanExpression

Rule to check for complex boolean expressions

src/main/java/com/cflint/plugins/core/ComplexBooleanExpressionChecker.java

@@ -0,0 +1,82 @@
+package com.cflint.plugins.core;
+
+import net.htmlparser.jericho.Element;
+import cfml.parsing.cfscript.CFExpression;
+import cfml.parsing.cfscript.CFBinaryExpression;
+import cfml.parsing.cfscript.script.CFScriptStatement;
+import cfml.parsing.cfscript.script.CFIfStatement;
+import cfml.parsing.cfscript.script.CFExpressionStatement;
+
+import com.cflint.BugInfo;
+import com.cflint.BugList;
+import com.cflint.plugins.CFLintScannerAdapter;
+import com.cflint.plugins.Context;
+
+public class ComplexBooleanExpressionChecker extends CFLintScannerAdapter {
+	final String severity = "WARNING";
+
+	protected int complexThreshold = 10;
+
+	@Override	
+	public void expression(final CFExpression expression, final Context context, final BugList bugs) {
+		if (expression instanceof CFBinaryExpression) {
+			String code = expression.Decompile(0).toLowerCase();
+
+			if (isComplex(code, complexThreshold)) {
+				int lineNo = ((CFBinaryExpression) expression).getLine() + context.startLine() - 1;
+
+				complexBooleanExpression(lineNo, context, bugs);
+			}
+		}
+	}
+
+	@Override
+	public void element(final Element element, final Context context, final BugList bugs) {
+		String tag = element.getName();
+
+		if (tag.equals("cfif")) { 
+			String content = element.getStartTag().getTagContent().toString();
+
+			if (isComplex(content, complexThreshold)) {
+				int lineNo = element.getSource().getRow(element.getBegin());
+
+				complexBooleanExpression(lineNo, context, bugs);
+			}
+		}
+	}
+
+	protected boolean isComplex(final String code, final int complexThreshold) {
+		int noAnds = noSubstrings(code, " && ") + noSubstrings(code, " and ");
+		int noOrs = noSubstrings(code, " || ") + noSubstrings(code, " or ");
+		int noNots = noSubstrings(code, " ! ") + noSubstrings(code, " not ");
+
+		// This is just a rough heuristic
+		// lots of and's or or's on their own is usually easy to understand
+		// but combine them together and it gets hard to understand very quickly 
+		return (noAnds * noOrs + noNots + noAnds + noOrs) > complexThreshold;
+	}
+
+	protected int noSubstrings(String string, String substring) {
+		int lastIndex = 0;
+		int count = 0;
+
+		while (lastIndex != -1) {
+		    lastIndex = string.indexOf(substring, lastIndex);
+
+		    if (lastIndex != -1) { 
+		        count ++;
+		        lastIndex += substring.length();
+		    }
+		}
+
+		return count;
+	}
+
+	public void complexBooleanExpression(final int lineNo, final Context context, final BugList bugs) {
+		bugs.add(new BugInfo.BugInfoBuilder().setLine(lineNo).setMessageCode("COMPLEX_BOOLEAN_CHECK")
+			.setSeverity(severity).setFilename(context.getFilename())
+			.setMessage("Boolean expession at " + lineNo + " is too complex. Consider simplifying or moving to a named method.")
+			.build());
+	}
+
+}

src/main/resources/cflint.definition.xml

@@ -193,4 +193,9 @@
 			<severity>INFO</severity>
 		</message>
 	</ruleImpl>
+	<ruleImpl name="ComplexBooleanExpressionChecker" className="ComplexBooleanExpressionChecker">
+		<message code="COMPLEX_BOOLEAN_CHECK">
+			<severity>WARNING</severity>
+		</message>
+	</ruleImpl>
 </CFLint-Plugin>

src/test/java/com/cflint/TestComplexBooleanExpressionChecker.java

@@ -0,0 +1,65 @@
+package com.cflint;
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.IOException;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import cfml.parsing.reporting.ParseException;
+
+import com.cflint.config.CFLintPluginInfo.PluginInfoRule;
+import com.cflint.config.CFLintPluginInfo.PluginInfoRule.PluginMessage;
+import com.cflint.config.ConfigRuntime;
+import com.cflint.plugins.core.ComplexBooleanExpressionChecker;
+
+public class TestComplexBooleanExpressionChecker {
+
+	private CFLint cfBugs;
+
+	@Before
+	public void setUp() {
+		final ConfigRuntime conf = new ConfigRuntime();
+		final PluginInfoRule pluginRule = new PluginInfoRule();
+		pluginRule.setName("ComplexBooleanExpressionChecker");
+		conf.getRules().add(pluginRule);
+		final PluginMessage pluginMessage = new PluginMessage("COMPLEX_BOOLEAN_CHECK");
+		pluginMessage.setSeverity("WARNING");
+		cfBugs = new CFLint(conf, new ComplexBooleanExpressionChecker());
+	}
+
+	@Test
+	public void testBooleanExpressionInScript() throws ParseException, IOException {
+		final String scriptSrc = "<cfscript>\r\n"
+			+ "if (a && b || c && d || e && f) {\r\n"
+			+ "	c = 1;\r\n"
+			+ "}\r\n"
+			+ "else if (a or not b and not a or b and not c) {\r\n"
+			+ "	c = 1;\r\n"
+			+ "}\r\n"
+			+ "</cfscript>";
+
+		cfBugs.process(scriptSrc, "test");
+		final List<BugInfo> result = cfBugs.getBugs().getBugList().values().iterator().next();
+		assertEquals(2, result.size());
+		assertEquals("COMPLEX_BOOLEAN_CHECK", result.get(0).getMessageCode());
+		assertEquals(2, result.get(0).getLine());
+		assertEquals("COMPLEX_BOOLEAN_CHECK", result.get(1).getMessageCode());
+		assertEquals(5, result.get(1).getLine());
+	}
+
+	@Test
+	public void testBooleanExpressionInTag() throws ParseException, IOException {
+		final String tagSrc = "<cfset a = 23>\r\n"
+			+ "<cfset a = a and not b or c and d or not e and f>";
+
+		cfBugs.process(tagSrc, "test");
+		final List<BugInfo> result = cfBugs.getBugs().getBugList().values().iterator().next();
+		assertEquals(1, result.size());
+		assertEquals("COMPLEX_BOOLEAN_CHECK", result.get(0).getMessageCode());
+		assertEquals(2, result.get(0).getLine());
+	}
+
+}