Postfix calculator

Question

As part of my learning Java I'm doing various tasks I set myself - one such task is a postfix calculator. It seems to work with all tests I've given it, and I would really appreciate some review of the code I've created to complete this, both logic and style wise:

package com.VortixDev.PostfixInterpreter;

import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class PostfixInterpreter {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);

        String postfix = scan.nextLine();

        scan.close();

        postfix = calculate(postfix);

        System.out.println(postfix);
    }

    public static String calculate(String postfix) {
        String result = postfix;
        String initialPostfix;

        do {
            initialPostfix = result;

            do {
                postfix = result;

                result = processPostfix(result, '/');
            } while (result != postfix);

            do {
                postfix = result;

                result = processPostfix(result, '*');
            } while (result != postfix);

            do {
                postfix = result;

                result = processPostfix(result, '+');
            } while (result != postfix);

            do {
                postfix = result;

                result = processPostfix(result, '-');
            } while (result != postfix);
        } while (initialPostfix != result);

        return result;
    }

    private static String processPostfix(String postfix, char operator) {
        String numberMatch = "(\\-?\\d+(?:\\.\\d+)?)";

        Matcher matcher = Pattern.compile(numberMatch + " " + numberMatch + " \\" + operator).matcher(postfix);

        boolean findMatch = matcher.find();

        while (findMatch) {
            String match = matcher.group(0);

            double firstValue = Double.parseDouble(matcher.group(1));
            double secondValue = Double.parseDouble(matcher.group(2));
            double resultValue;

            switch (operator) {
                case '/':
                    resultValue = firstValue / secondValue;

                    break;
                case '*':
                    resultValue = firstValue * secondValue;

                    break;
                case '+':
                    resultValue = firstValue + secondValue;

                    break;
                case '-':
                    resultValue = firstValue - secondValue;

                    break;
                default:
                    return postfix;
            }

            String result = String.valueOf(resultValue);

            postfix = postfix.replace(match, result);

            findMatch = matcher.find();
        }

        return postfix;
    }
}

Answer 1

Why not creating one Operator interface with a child for each valid operation (+,-;*,/). You can also introduce an Operand and make it parent of Operator. So that you can do :

Double result = new Addition(new Operand(2), new Multiplication(new Operand(3), new Operand(3)); // 2 + (3 * 2)

With some syntax sugar and a parser that split on operators you should be able to build a nice OO version of your PostfixInterpreter.

Edit, argument in favor of many simple classes.

I agree that having many simple classes can seems overkill. But It will help you to create more robust systems and decompose the problem. In your case, instead of just "parsing a string", you can view a postfix expression that is sequence of words :

class Expression implements Iterable<Word>

Then evaluating an expression in simple as reducing it. You just need a stack to hold the values:

Stack<Word> stack = new Stack<Word>();
for (Word word : expression){
  word.evaluate(stack);
}
return stack.peek(); // More controls may be required.

I think it is still simple and comprehensible. And now you just have to implements Word for all cases and you quickly distinct the constants and the functions.

• Const is simple as pushing his value to the stack.
• Function pop his parameters from the stack and push the result (which will be a Const.

In a simple arithmetic context, you have one Const which is a Number and set of BinaryFunction : Addition, Multiplication, Division ...

At this stage you can test individually all functions and also test that your interpreter is able to reduce a sequence of Words.

PostfixInterpreter target = new PostfixInterpreter();
assertThat(target.evaluate(new Number(1), new Number(2), new Addition())
    .isEqualTo(new Number(1 + 2));
// And more complex calculations

It is a lot of new but you have proved that the interpreter works fine.

Now, you just have to parse the string. Which can be simple as exploding on white spaces and instantiate the class that match a word. Exploding the string as words is a simple scanner :

class ExpressionScanner implements Iterable<String>

And parsing is just a mapping of a String to a Word.

class ExpressionParser implements Iterable<Word> {
  private final ExpressionScanner scanner;

  public boolean hasNext() {
    return scanner.hasNext();
  }

  public Word next() {
    String word = scanner.next();
    return Parser.parse(word);
  }
}

The Parser can use regex to identify the token. In my case I would create one abstract class so that all implementations of Word can register they own parser to a registry.

abstract class Parser {
  private static final List<Parser> REGISTRY = new ArrayList<>();
  static {
    REGISTRY.add(Addition.PARSER);
    // ...
  }
  public static Word parse(String token) {
    for (Parser parser : REGISTRY) {
      if ( parser.accept(token) ) {
        return parser.apply(token);
      }
    }
    return null; // Bad idea, you should throw or wrap in Optional
  }


  private final Pattern pattern;
  protected Parser(Pattern pattern) {
    this.pattern = pattern;
  }

  boolean accept(String token) {
    return pattern.matcher(token).matches();
  }

  abstract Word apply(String token);
}

If you are using Java 8 you can integrate the java.util.function elements when applicable. Like in Parser that is a combination of Predicate<String> and Function<String, Word> as Parser. And in your functions hierarchy.

The exception handling must be implement like in ExpressionScanner and ExpressionParser that can work together to provide detailed information on a parsing exception.

class ExpressionParser {

  Word next() {
    Word parsed = ...
    if ( parsed==null ) {
      throw new ExpressionParsingException(expression, word, scanner.position());
    } else {
      return parsed;
    }
  }
}

Answer 2

Too much whitespace (particularly empty lines) for my liking. Try to use empty lines to group code into small clusters of code that belong together. For example, you wrote:

switch (operator) {
    case '/':
        resultValue = firstValue / secondValue;

    break;
    case '*':
        resultValue = firstValue * secondValue;

        break;

Whereas, imo, it would be easier to read like this:

switch (operator) {
    case '/':
        resultValue = firstValue / secondValue;
        break;

    case '*':
        resultValue = firstValue * secondValue;
        break;