Simple calculator for an interview

Question

Question: Simple Calculator

Input loaded from file. Instructions can be any binary operators. Ignore math precedence. Last input is apply and a number e.g. "apply 5". Calc is then initialized with that number and previous instructions are applied to it.

Also: There is no requirement to cater for invalid input.

Example 1:

[Input from file]

add 2
multiply 3
apply 3

[Output to screen]

15

[Explanation]

(3 + 2) * 3 = 15

Example 2:

[Input from file]

multiply 9
apply 5

[Output to screen]

45

[Explanation]

5 * 9 = 45

Example 3:

[Input from file]

apply 1

[Output to screen]

1

I've submitted the following code (with unit tests). I think the mistakes I made were:

1. I think the design was too complicated because I created a processor interface, which could have multiple implementations. Maybe the command class was a bad idea.

2. Leaving this method in Operator, but not using it: abstract BigDecimal calculate(BigDecimal x); and not providing a Boolean method in the abstract class to determine which of these methods should be used by a client class.

3. The getSymbol method in FileCalculationConsumer. If the design was generic, I should not have hard coded that method.

4. Badly-named files?

There may other places where I went wrong. Any feedback is appreciated.

class FileCalculationConsumer

import java.io.File;
import java.io.FileNotFoundException;
import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Queue;
import java.util.Scanner;

/**
 * Main Class for the File based Calculation Consumer. Calculation instructions 
 * are consumed in batches.
 *  
 * The FileCalculationConsumer class provides methods to consume Calculation
 * Instructions from a flat file. Calculation instructions are mapped to 
 * Command objects. Processing of the command objects is delegated to a
 * processor. 
 * 
 *
 */
public class FileCalculationConsumer implements CalculationConsumer {
    private String inputPath="";
    private Scanner in;
    private int batchSize = 100;
    int batchCount=0;

    private CommandProcessor processor;

    private FileCalculationConsumer(String inputPath, CommandProcessor processor) {
        this.inputPath = inputPath;
        try {
            this.in = new Scanner(new File(inputPath));
        } catch (FileNotFoundException e) {
            System.err.println("Input file not found at path: " + inputPath);
            return;
        }
        if(processor == null) {
            System.err.println("CommandProcessor must not be null");
            return;
        }
        this.processor = processor;
    }
    /**
     * Initialises the Consumer with the input file path and the processor.
     * 
     * @param inputPath The path of the input file containing Calculation instructions
     * @param processor The processor for the calculation command objects
     * @param batchSize The size of each batch of instructions to consume
     * @throws IllegalAccessException 
     * @throws BatchSizeInvalidException 
     */
    public FileCalculationConsumer(String inputPath, CommandProcessor processor,int batchSize) throws BatchSizeInvalidException {
        this(inputPath,processor);
        if(batchSize<1) {
            throw new BatchSizeInvalidException("Batchsize must be larger than zero");
        }
        this.batchSize = batchSize;
    }

    /* (non-Javadoc)
     * @see CalculationConsumer#consume()
     */
    @Override
    public void consume() {
        Queue<Command> commands = readCommandBatch();
        processor.process(commands);
        if(commands.size() == batchSize) {
            consume();
        }
        else {
            in.close();
        }
    }

    /**
     * 
     * Reads in a batch of Calculation instructions and creates a new 
     * Command object for each one.
     * 
     * @return A queue of commands, which has the size as configured by 
     * the batchsize variable.
     */
    public LinkedList<Command> readCommandBatch() {
        LinkedList<Command> result = new LinkedList<>();
        int count=0;

        while(in.hasNext() && count < batchSize) {
            String operation = getSymbol(in.next());
            BigDecimal operand = in.nextBigDecimal();
            Command command = new Command(operation,operand);
            result.add(command);
            if(in.hasNextLine())
                in.nextLine();
            count++;
        }
        return result;
    }

    private String getSymbol(String operation) {
        if(operation.equals("add"))
            return "+";
        else if (operation.equals("multiply")) {
            return "*";
        }
        else if (operation.equals("apply")) {
            return "=";
        }
        else {
            throw new IllegalArgumentException(operation + "Is an invalid operator");
        }
    }

    public String getInputPath() {
        return inputPath;
    }

    /**
     * Main entry point for the File based Calculation Consumer. Calculation instructions 
     * are consumed in batches.
     *
     * @param args
     */
    public static void main(String[] args) {
        if(args.length!=2) {
            System.err.println("Usage: Java FileCalculationConsumer <filepath> <batchsize>");
            return;
        }
        int batchSize=0;
        String inputFile=args[0];
        String batchSizeString=args[1];
        try{
            batchSize = Integer.parseInt(batchSizeString);
        }
        catch(NumberFormatException e) {
            System.err.println("Usage: Batchsize must be numeric");
            return;
        }
        consume(batchSize, inputFile);
    }
    private static void consume(int batchSize, String inputFile) {
        List<Operator> operators = createOperators();
        CommandProcessor processor = new DefaultCommandProcessor(new Calculator(operators));
        CalculationConsumer consumer=null;
        try {
            consumer = new FileCalculationConsumer(inputFile,processor,batchSize);
        } catch (BatchSizeInvalidException e) {
            System.err.println("BatchSize must be greater than zero");
            return;
        }
        consumer.consume();
    }

    private static List<Operator> createOperators() {
        List<Operator> operators = new ArrayList<>();
        Operator multiply = Operator.createMultiplyOperator();
        Operator add = Operator.createAddOperator();    
        operators.add(multiply);
        operators.add(add);
        return operators;
    }
}

interface CalculationConsumer

/**
* A consumer for Calculation Instructions.
*
*/
public interface CalculationConsumer {

    /**
     * Consumes Calculation Instructions
     */
    public abstract void consume();

}

class DefaultCommandProcessor

import java.util.ArrayDeque;
import java.util.Deque;
import java.util.Queue;


/**
 * The DefaultCommandProcessor class provides the default implementation of the 
 * CommandProcessor. This processor reorders the commands, so that the Apply command
 * is first in the queue of the commands. Calculation is delegated to the Calculator 
 * class.
 *
 */
public class DefaultCommandProcessor implements CommandProcessor {

    private Calculator calc;
    private Deque<Command> deque = new ArrayDeque<Command>();

    /**
     * Initialises this processor with a specific calculator.
     * @param calc
     */
    public DefaultCommandProcessor(Calculator calc) {
        this.calc = calc;
    }

    /**
     * @param deque
     */
    private void calculate(Deque<Command> deque) {
        for (Command command : deque) {
            calc.calculate(command);
        }
    }

    @Override
    public void process(Command command) {
        if(command.getOperation().equals("=")) {
            deque.addFirst(command);
            calculate(deque);
            calc.displayResult();
            calc.resetState();
            deque.clear();
        }
        else {
            deque.add(command);
        }
    }

    @Override
    public void process(Queue<Command> commands) {
        for (Command command : commands) {
            process(command);
        }
    }
}

interface CommandProcessor

import java.util.Queue;


public interface CommandProcessor {

    /**
     * Processes a single command object
     * 
     * @param command
     */
    void process(Command command);

    /**
     * Processes a queue of command objects
     * 
     * @param commands
     */
    void process(Queue<Command> commands);
}

class Command

import java.math.BigDecimal;


/**
 * The Command class encapsulates a Calculation request. 
 * This class is immutable.
 * 
 * Example operation "+"
 * Example operand 2
 * 
 *
 */
public class Command {

    private String operation;
    private BigDecimal operand;

    /**
     * Initializes the immutable Command 
     * @param operation The operation to perform
     * @param operand The associated operand
     */
    public Command(String operation, BigDecimal operand) {
        this.operation = operation;
        this.operand = operand;
    }

    /**
     * Accessor of the Operation of a Command
     * @return a String representation of the operator
     */
    public String getOperation() {
        return operation;
    }

    /**
     * Accessor for the operand, which is the value that this 
     * operator is applied to.
     * @return
     */
    public BigDecimal getOperand() {
        return operand;
    }
}

class Calculator

import java.math.BigDecimal;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
 * The Calculator class provides a stateful calculator, with
 * operators passed in at construction.
 *   
 *
 */

public class Calculator {

    HashMap<String, Operator> operators = new HashMap<>();
    private BigDecimal operand1=new BigDecimal(0);

    /**
     * Initializes a new Calculator with the operators provided.
     * 
     * @param operators The operators this Calculator can use to 
     * perform mathematical operations.
     */
    public Calculator(List<Operator> operators) {
        for (Operator operator : operators) {
            this.operators.put(operator.getSymbol(),operator);
        }
    }

    /**
     * Handles a command. If the command is an equals command, 
     * the state of the calculator is updated to reflect this. 
     * Otherwise the corresponding mathematical 
     * operation is performed for this command and then the calculator state is updated 
     * to reflect this operation.
     * 
     * @param com
     */
    public void calculate(Command com) {
        if(com.getOperation().equals("=")) { 
            operand1=com.getOperand();
            return;
        }
        else {
            String operation = com.getOperation();
            Operator op = operators.get(operation);
            operand1 = op.calculate(operand1,com.getOperand());
        }
    }

    /**
     * Accessor for the operators set for this Calculator.
     * 
     * @return An unmodifiable collection of immutable operators. 
     */
    public Map<String, Operator> getOperators() {
        return Collections.unmodifiableMap(operators);
    }

    /**
     * Displays the current state of the calculator to 
     * the console
     */
    void displayResult() {
        System.out.println(operand1.toString());
    }

    /**
     * Displays the current state of the calculator
     * @return
     */
    public BigDecimal getOperand1() {
        return operand1;
    }

    /**
     * Resets the state of the calculator 
     */
    public void resetState() {
        operand1= new BigDecimal(0);
    }
}

interface CalculationConsumer

/**
 * A consumer for Calculation Instructions.
 *
 */
public interface CalculationConsumer {

    /**
     * Consumes Calculation Instructions
     */
    public abstract void consume();

    }

class operator

import java.math.BigDecimal;


/**
 * Provides an immutable abstract type for a binary
 * mathematical operator
 * 
 *
 */
public abstract class Operator {

    private String symbol;

    public Operator(String symbol) {
        this.symbol=symbol;
    }

    abstract BigDecimal calculate(BigDecimal x, BigDecimal y);

    abstract BigDecimal calculate(BigDecimal x);

    public String getSymbol() {
        return symbol;
    }

    static Operator createAddOperator() {
        Operator add = new Operator("+") {

            @Override
            BigDecimal calculate(BigDecimal x, BigDecimal y) {
                return x.add(y);
            }

            @Override
            BigDecimal calculate(BigDecimal x) {
                throw new UnsupportedOperationException();
                }};
        return add;
    }

    static Operator createMultiplyOperator() {
        Operator multiply = new Operator("*") {

            @Override
            BigDecimal calculate(BigDecimal x, BigDecimal y) {
                return x.multiply(y);
            }

            @Override
            BigDecimal calculate(BigDecimal x) {
                throw new UnsupportedOperationException();
            }};
        return multiply;
    }

}

class FileCalculationConsumerTest

package test;
import static org.junit.Assert.assertEquals;
import BatchSizeInvalidException;
import CalculationConsumer;
import Command;
import FileCalculationConsumer;

import java.io.FileNotFoundException;
import java.util.Queue;

import org.junit.Before;
import org.junit.Test;

public class FileCalculationConsumerTest extends BaseTest {
    private static final String UNITTEST_TXT = "UnitTest.txt";
    private static final int BATCH_SIZE = 10;
    FileCalculationConsumer consumer;

    @Before
    public void setup() throws FileNotFoundException, BatchSizeInvalidException {
        consumer = new FileCalculationConsumer(UNITTEST_TXT,processor,BATCH_SIZE);
    }

    @Test
    public void testCreate() throws FileNotFoundException {
        assertEquals(UNITTEST_TXT,consumer.getInputPath());
    }

    public void testCreateWithInvalidFilePath() throws BatchSizeInvalidException {
         CalculationConsumer consumer = new FileCalculationConsumer("ex.txt",processor,3);
         assertEquals("Input file not found at path: ex.txt", out.toString().trim());
    }

    @Test(expected=IllegalArgumentException.class)
    public void testConsumeInvalidOperator() throws BatchSizeInvalidException {
         FileCalculationConsumer consumer = new FileCalculationConsumer("InvalidOperatorTest.txt",processor,3);
         consumer.readCommandBatch();
    }

     @Test
    public void testProcess() {
        consumer.consume();
        assertEquals("15", out.toString().trim());
    }

    @Test
    public void testReadCommandBatch() throws FileNotFoundException, BatchSizeInvalidException {
        consumer = new FileCalculationConsumer(UNITTEST_TXT,processor,2);
        Queue<Command> batch = consumer.readCommandBatch();
        assertEquals(2,batch.size());
        batch = consumer.readCommandBatch();
        assertEquals(1,batch.size());
    }     
}

Answer 1

It looks to me like you went all over the place here, both over engineering things, and also inventing some additional requirements that don't seem to be in the problem statement.

The calculator itself is pretty simple - start with an input, apply operations until you run out, emit the answer. I'd probably go to generics right away -- intending to test with Integers or Longs, but if you want to use BigDecimal, that would be fine too. It's a good place to have a discussion about requirements and tradeoffs

interface Operation<T> {
    T calculate(T input);
}

class Calculator<T> {
    final List<Operation<T> operations;

    Calculator(Operation<T> operations) {
        this.operations = operations;
    }

    T calculate(T input) {
        T result = input;
        for(Operation<T> op : operations) {
            result = op.calculate(result);
        }
        return result;
    }
}

From here, the next step is to create the operation List. The input file is a set of instructions, that either tell us to build more, or to output the answer. That sounds to me like an Interpreter

class Interpreter<T> {
    void run(Iterable<String> instructions) {
        List<Operation<T>> operations = Lists.newArrayList();

        for(String instruction : instructions) {
            if (instruction.startsWith("apply")) {
                T input = getInput(instruction);

                Calculator<T> calc = new Calculator(operations);
                T output = calc.calculate(input);

                System.out.println(String.valueOf(output));

                break;
            } else {
                operations.add(parse(instruction));
            }
        }
    }
}

I'm not a big fan of branch based behavior, so I might shuffle this up a bit....

class Interpreter<T> {
    void run(Iterable<String> instructions) {
        List<Operation<T>> operations = Lists.newArrayList();

        Command parse = new Parser(operations);
        Command execute = new Execute(operations);

        for(String instruction : instructions) {
            if (instruction.startsWith("apply")) {
                execute.run(instruction);
                break;
            } else {
                parse.run(instruction);
            }
        }
    }
}

You get a somewhat cleaner design if you realize that you get the right answer if "apply" produces an operation that returns its input unchanged.

class Interpreter<T> {
    void run(Iterable<String> instructions) {
        List<Operation<T>> operations = Lists.newArrayList();

        Parser parser = new Parser(operations);

        for(String instruction : instructions) {
            parser.parse(instruction);
        }
        String lastInstruction = parser.getLastInstruction();

        Execute execute = new Execute(operations);
        execute.run(lastInstruction);
    }
}

The Parser's job is to figure out which factory is responsible for creating the operation specified by each instruction.

class Parser<T> {
    // the factories are going to give us something that implements
    // Operation<T>, so extends is right
    private final Iterable<Factory<? extends Operation<T>>> factories ;

    // the parser is putting items into the list, so super is the
    // right answer, in case a client hands us a List<Object>
    private final List<? super Operation<T>> operations;

    Parser(Iterable<Factory<? extends Operation<T>>> factories, List<Operation<T>> operations) {
        this.factories = factories;
        this.operations = operations;
    }

    public void parse(String instruction) {
        setLastInstruction(instruction);

        for(Factory<Operation<T>> factory : factories) {
            if (factory.match(instruction)) {
                operations.add(factory.create(instruction));
                break;
            }
        }
    }
    // ...
}


class MultiplicationFactory<T> implements Factory<MultiplicationOperation<T>> {
    static final String keyword = "multiply ";

    final ConvertStringTo<T> convertor;

    // ...

    public boolean match(String instruction) {
        return instruction.startsWith(keyword);
    }

    public MultiplicationOperation<T> create(String instruction) {

        T operand = convertor.convert(instruction.substring(keyword.length());

        return new MultiplicationOperation(operand);
    }

    //...
}

You could go really overboard on the instruction to operation problem; at the low end, you call a bunch of string operations. You can split the string into tokens, and then use the first token to discover how to interpret the rest. You can use a set of regular expressions. You can build a grammar.....

Answer 2

my 2 cents:

• I am not sure why you split the constructor over 2 methods. Looks awkward.
• You store inputPath in FileCalculationConsumer, you even have a getter, but you never do anything with after you create the Scanner
• Why would you initialize batch size to 100 if you get it from a parameter?
• It seems that if there are less than batchSize commands, the program does quietly nothing?
• You have far too many methods called consume, I get lost trying to track how your code works, whereas this should be pretty simple

Note, you should probably reformat your question, to separate the class files and put the filenames in bold on top of your code..