3
\$\begingroup\$

I was reading this post and saw quite few improvements. So I tried to rewrite it using Java 8 features. Any improvements you can spot, please feel free to mention.

public static void main(String[] args) {

    boolean runAgain = false;
    Double result = .0;
    List<Double> inputs = new ArrayList<Double>();
    Scanner scanner = new Scanner(System.in);
    do {
        System.out.print("Enter the first number : ");
        inputs.add(scanner.nextDouble());
        System.out.print("Enter the second number : ");
        inputs.add(scanner.nextDouble());
        System.out.print("Enter the operator : ");
        String op = scanner.next();

        result = calculateAnswer(inputs, op);
        System.out.println("Answer : "+result);

        System.out.print("Do you want to try again?[Y/N] : ");
        runAgain = scanner.next().equals("Y")?true : false;

    } while (runAgain);

    System.out.println("Program finished!");
    scanner.close();
}

static Double calculateAnswer(List<Double> inputs, String operator){

    Operations[] ops = Operations.values();
    Operations correctOp = Arrays.asList(ops).stream()
            .filter(op -> op.getSymbol().equals(operator))
            .findFirst().get();

    return correctOp.getResult(inputs.get(0), inputs.get(1));
}

enum Operations{
    ADD(){
        @Override
        String getSymbol() {
            return "+";
        }
        @Override
        Double getResult(Double d1, Double d2) {
            return d1 + d2;
        }
    },SUB{
        @Override
        String getSymbol() {
            return "-";
        }
        @Override
        Double getResult(Double d1, Double d2) {
            return d1 - d2;
        }
    },MUL{
        @Override
        String getSymbol() {
            return "*";
        }
        @Override
        Double getResult(Double d1, Double d2) {
            return d1 * d2;
        }
    },DIV{
        @Override
        String getSymbol() {
            return "/";
        }
        @Override
        Double getResult(Double d1, Double d2) {
            return d1 / d2;
        }
    };

    abstract Double getResult(Double d1, Double d2);

    abstract String getSymbol();
}
\$\endgroup\$
  • \$\begingroup\$ You probably want to clear inputs at some point or you won’t be able to make a different calculation \$\endgroup\$ – poke Jul 24 '16 at 15:46
3
\$\begingroup\$

Embrace lambda expressions!

Your Operations is a lot more complicated that it needs to be. First of all, I would rename it Operation instead of Operations: the class does not represent multiple operations but a single one.

An Operation has 2 members: the symbol and the concrete operation it is performing. As such, consider adding 2 fields to your class symbol and operation. They will be initialized using a custom constructor. For the operation, all of them will be taking as input 2 double values, and returning a double that is the result of the calculation. This can be represented with the DoubleBinaryOperator class. Consider this:

enum Operation {

    ADD("+", (d1, d2) -> d1 + d2), // or Double::sum
    SUB("-", (d1, d2) -> d1 - d2),
    MUL("*", (d1, d2) -> d1 * d2),
    DIV("/", (d1, d2) -> d1 / d2);

    private final String symbol;
    private final DoubleBinaryOperator operator;

    private Operation(String symbol, DoubleBinaryOperator operator) {
        this.symbol = symbol;
        this.operator = operator;
    }

    public double getResult(double d1, double d2) {
        return operator.applyAsDouble(d1, d2);
    }

    public String getSymbol() {
        return symbol;
    }

}

The great advantage is that the code is now a lot shorter, and more readable. Instances of the concrete operations are created using a lambda expression.

Prefer primitives when possible

Your code is working on Double objects. In fact, you could just work with the primitive type double: simpler, no auto-boxing that may impact performance is performed. You should avoid to create unnecessary objects when possible.

Careful with Optional.get()

To retrieve the operation from its symbol, the code is looping over all the possible operations, filtering the one having the correct symbol and returning it. However, if there are no operations with this symbol, it will fail with a NoSuchElementException from Optional#get().

The issue is that it is "hidden" in the code. When reading it, you can easily not spot this fact. Instead of relying on this behaviour, you should express in your code what you want to happen when no operations were found. You could default to another operation with orElse, but most likely you will want to fail with an exception. And for that you can use orElseThrow. The advantage is that it documents clearly in the code what is expected to happen when no operations are found.

Operation correctOp =
    Arrays.stream(Operation.values())
          .filter(op -> op.getSymbol().equals(operator))
          .findFirst()
          .orElseThrow(() -> new IllegalArgumentException("No operation found for operator " + operator));

The end result is the same: an exception will be thrown. But in this example, the reader and the maintainer of the code clearly sees that this case was thought of. Also, it provides a more meaningful exception message for the user.

Note that I replaced the creation of the Stream with Arrays.stream(array): there is no need to first convert to a list to create a Stream, you can do it directly on an array.

Clear parameters to functions

The method calculateAnswer currently takes as parameter a List<Double> inputs for the inputs by the user. Why? This is awkward: it would imply the function could be called with an empty list, or a list with 1 element... and the code will fail trying to access an out of bounds index since it expects at least 2 elements in the list.

In fact, the method only takes 2 inputs, so instead of wrapping them in a list, pass them as 2 parameters:

double calculateAnswer(double firstInput, double secondInput, String operator)

This also means you need to refactor a bit the main code: don't use a List<Double> but store each number given by the user in 2 separate local variables.

Boolean arithmetic

Don't do

runAgain = scanner.next().equals("Y")?true : false;

It is both clearer and shorter to just have

runAgain = scanner.next().equals("Y");
\$\endgroup\$
  • \$\begingroup\$ woah, that kind of a review! Thanks mate. Appreciate it! \$\endgroup\$ – Jude Niroshan Jul 25 '16 at 5:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.