Refactored the COND built-in function

src/function/ArgumentValidator.java

@@ -50,26 +50,25 @@ public class ArgumentValidator {
 
     public void validate(Cons argumentList) {
         validateListNotDotted(argumentList);
-
+        validateListLength(argumentList);
-        if (containsTooFewArguments(argumentList))
-            throw new TooFewArgumentsException(functionName, argumentList);
-        else if (containsTooManyArguments(argumentList))
-            throw new TooManyArgumentsException(functionName, argumentList);
-
         validateArgumentTypes(argumentList);
     }
 
     private void validateListNotDotted(Cons argumentList) {
-        Cons currentCons = argumentList;
+        SExpression next = argumentList.getCdr();
-        SExpression nextCons = argumentList.getCdr();
 
-        while (!nextCons.nullp()) {
+        for (Cons current = argumentList; next.consp(); next = current.getCdr())
-            if (!nextCons.consp())
+            current = (Cons) next;
+
+        if (!next.nullp())
             throw new DottedArgumentListException(functionName, argumentList);
-
-            currentCons = (Cons) nextCons;
-            nextCons = currentCons.getCdr();
     }
+
+    private void validateListLength(Cons argumentList) {
+        if (containsTooFewArguments(argumentList))
+            throw new TooFewArgumentsException(functionName, argumentList);
+        else if (containsTooManyArguments(argumentList))
+            throw new TooManyArgumentsException(functionName, argumentList);
     }
 
     private boolean containsTooFewArguments(Cons argumentList) {
@@ -101,6 +100,11 @@ public class ArgumentValidator {
         return trailingArgumentType.isInstance(remainingArgument);
     }
 
+    public void validateListIsNotNil(Cons list) {
+        if (list.nullp())
+            throw new BadArgumentTypeException(functionName, Nil.getUniqueInstance(), Cons.class);
+    }
+
     public static class TooFewArgumentsException extends LispException {
 
         private static final long serialVersionUID = 1L;

src/function/builtin/COND.java

@@ -1,53 +1,63 @@
 package function.builtin;
 
-import function.LispFunction;
+import function.*;
 import sexpression.*;
 
 public class COND extends LispFunction {
 
-    public SExpression call(Cons argList) {
+    private ArgumentValidator argumentValidator;
-        if (argList.nullp()) {
+
-            // return NIL if there are were no arguments passed to COND
+    public COND() {
-            return Nil.getUniqueInstance();
+        this.argumentValidator = new ArgumentValidator("COND");
+        this.argumentValidator.setEveryArgumentExpectedType(Cons.class);
     }
 
-        SExpression argCar = argList.getCar(); // first clause
+    public SExpression call(Cons argumentList) {
-        Cons argCdr = (Cons) argList.getCdr(); // list of remaining clauses
+        argumentValidator.validate(argumentList);
 
-        // make sure the first clause is a list and is not NIL
+        return callTailRecursive(argumentList);
-        if (argCar.consp()) {
+    }
-            Cons clause = (Cons) argCar;
+
+    private SExpression callTailRecursive(Cons argumentList) {
+        if (argumentList.nullp())
+            return Nil.getUniqueInstance();
+
+        Cons clause = getFirstClauseAndValidateNotNil(argumentList);
+        Cons remainingClauses = (Cons) argumentList.getCdr();
         SExpression test = EVAL.eval(clause.getCar());
 
-            if (test != Nil.getUniqueInstance()) {
+        if (isTestSuccessful(test))
-                // the car of this clause is true, so we evaluate its cdr
+            return evaluateResult(clause, test);
 
-                SExpression cdr = clause.getCdr();
+        return callTailRecursive(remainingClauses);
-                SExpression retval = test;
-
-                // evaluate all the S-expressions in the cdr of the clause
-                while (cdr.consp()) {
-                    retval = EVAL.eval(((Cons) cdr).getCar());
-                    cdr = ((Cons) cdr).getCdr();
     }
 
-                // return the value of the last S-expression evaluated
+    private Cons getFirstClauseAndValidateNotNil(Cons argumentList) {
-                return retval;
+        Cons firstClause = (Cons) argumentList.getCar();
+        argumentValidator.validateListIsNotNil(firstClause);
+
+        return firstClause;
     }
 
-            // the car of this clause is false, so we test any remaining
+    private boolean isTestSuccessful(SExpression test) {
-            // clauses
+        return test != Nil.getUniqueInstance();
-
-            // check if the list of remaining clauses is a list and is not NIL
-            if (argCdr.consp()) {
-                return call(argCdr);
     }
 
-            // there are no remaining clauses, so we return NIL
+    private SExpression evaluateResult(Cons clause, SExpression test) {
-            return Nil.getUniqueInstance();
+        SExpression lastResultValue = test;
+
+        for (SExpression result = clause.getCdr(); result.consp(); result = advanceCons(result))
+            lastResultValue = EVAL.eval(getCar(result));
+
+        return lastResultValue;
     }
 
-        throw new RuntimeException("COND: clause " + argCar + " should be a list");
+    private SExpression advanceCons(SExpression knownCons) {
+        return ((Cons) knownCons).getCdr();
+    }
+
+    private SExpression getCar(SExpression knownCons) {
+        return ((Cons) knownCons).getCar();
     }
 
     public boolean evaluateArguments() {

test/function/ArgumentValidatorTester.java

@@ -165,7 +165,8 @@ public class ArgumentValidatorTester {
         try {
             validator.validate(argumentList);
         } catch (BadArgumentTypeException e) {
-            e.getMessage();
+            assertNotNull(e.getMessage());
+            assertTrue(e.getMessage().length() > 0);
         }
     }
 
@@ -183,6 +184,13 @@ public class ArgumentValidatorTester {
         validator.validate(argumentList);
     }
 
+    @Test
+    public void DottedArgumentListException_HasCorrectSeverity() {
+        DottedArgumentListException e = new DottedArgumentListException("TEST", Nil.getUniqueInstance());
+
+        assertTrue(e.getSeverity() < ErrorManager.CRITICAL_LEVEL);
+    }
+
     @Test
     public void dottedArgumentListException_HasMessageText() {
         DottedArgumentListException e = new DottedArgumentListException("TEST", Nil.getUniqueInstance());
@@ -191,4 +199,14 @@ public class ArgumentValidatorTester {
         assertTrue(e.getMessage().length() > 0);
     }
 
+    @Test(expected = BadArgumentTypeException.class)
+    public void validateListNotNil_ThrowsExceptionWithNilValue() {
+        validator.validateListIsNotNil(Nil.getUniqueInstance());
+    }
+
+    @Test
+    public void validateListNotNil_DoesNotThrowExceptionWithNonNilValue() {
+        validator.validateListIsNotNil(new Cons(Nil.getUniqueInstance(), Nil.getUniqueInstance()));
+    }
+
 }

test/function/builtin/CONDTester.java

@@ -0,0 +1,77 @@
+package function.builtin;
+
+import static testutil.TestUtilities.*;
+
+import org.junit.Test;
+
+import function.ArgumentValidator.BadArgumentTypeException;
+
+public class CONDTester {
+
+    @Test
+    public void testCondWithNoArguments() {
+        String input = "(cond)";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("nil"));
+    }
+
+    @Test
+    public void testCondWithTrue() {
+        String input = "(cond (T))";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("T"));
+    }
+
+    @Test
+    public void testCondWithSingleExpression() {
+        String input = "(cond (T \"true\"))";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("\"true\""));
+    }
+
+    @Test
+    public void testCondWithMultipleExpressions() {
+        String input = "(cond ((= 1 2) 2) ((= 1 2) 2) ((= 1 1) 3))";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("3"));
+    }
+
+    @Test
+    public void testCondWithMultipleConditionsMatching_ReturnFirstOne() {
+        String input = "(cond ((= 1 1) 2) ((= 1 1) 3))";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("2"));
+    }
+
+    @Test
+    public void testCondWithMultipleConditionsMatching_OnlyEvaluatesFirstOne() {
+        String input = "(cond ((= 1 1) 2) ((= 1 1) x))";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("2"));
+    }
+
+    @Test
+    public void testCondWithMultipleResultValues_OnlyReturnsLast() {
+        String input = "(cond ((= 1 1) 2 3 4))";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("4"));
+    }
+
+    @Test
+    public void testCondWithNoConditionMatching_ReturnsNil() {
+        String input = "(cond ((= 1 2) T) ((= 1 3) T))";
+
+        assertSExpressionsMatch(evaluateString(input), parseString("nil"));
+    }
+
+    @Test(expected = BadArgumentTypeException.class)
+    public void testCondWithNilArgument_ThrowsException() {
+        evaluateString("(cond ())");
+    }
+
+    @Test(expected = BadArgumentTypeException.class)
+    public void testCondWithNonListArgument_ThrowsException() {
+        evaluateString("(cond o)");
+    }
+
+}