5
\$\begingroup\$

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;
    }
}
\$\endgroup\$
2
\$\begingroup\$

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;
    }
  }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! Whilst I like the idea of breaking it down even further, to get to a stage where I could use "new (operation)" I'd have to have already extracted the values and I personally wouldn't be a fan of creating a class for a simple arithmetical operation like multiplication. \$\endgroup\$ – VortixDev Aug 17 '16 at 13:04
  • \$\begingroup\$ Along the lines of this answer, maybe instead you could have an enum that has all the allowed operators in it? \$\endgroup\$ – kamoroso94 Aug 17 '16 at 13:43
2
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! Yeah, I see where you're coming from about the whitespace, though I personally like to separate unrelated statements with a line. \$\endgroup\$ – VortixDev Aug 17 '16 at 13:01
  • \$\begingroup\$ @VortixDev I think another school of thought is that unrelated statements should be in their own methods, for some definition of 'unrelated'... ;) \$\endgroup\$ – h.j.k. Aug 20 '16 at 5:55

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.