patternjavaMinor

Simple calculator for an interview

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

interviewsimplecalculatorfor

Problem

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]

[Explanation]

(3 + 2) * 3 = 15

Example 2:

[Input from file]

multiply 9
apply 5

[Output to screen]

[Explanation]

5 * 9 = 45

Example 3:

[Input from file]

apply 1

[Output to screen]

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

-
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.

-
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.

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

-
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

Solution

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 calculate(T input);
}

class Calculator {
    final List operations;

    Calculator(Operation operations) {
        this.operations = operations;
    }

    T calculate(T input) {
        T result = input;
        for(Operation 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 {
    void run(Iterable instructions) {
        List> operations = Lists.newArrayList();

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

                Calculator 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 {
    void run(Iterable instructions) {
        List> 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 {
    void run(Iterable instructions) {
        List> 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 {
    // the factories are going to give us something that implements
    // Operation, so extends is right
    private final Iterable>> factories ;

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

    Parser(Iterable>> factories, List> operations) {
        this.factories = factories;
        this.operations = operations;
    }

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

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

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

    final ConvertStringTo convertor;

    // ...

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

    public MultiplicationOperation 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.....

Code Snippets

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;
    }
}

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));
            }
        }
    }
}

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);
            }
        }
    }
}

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);
    }
}

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);
    }

    //...
}

Context

StackExchange Code Review Q#34026, answer score: 9

Revisions (0)

No revisions yet.