HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Infix to postix conversion

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
postixconversioninfix

Problem

I'm looking for code review, optimizations and best practices.

Also verifying complexity to be O(n), where n is the string length.

```
public final class InfixToPostfix {

private static final char PLUS = '+';
private static final char MINUS = '-';
private static final char MULTIPLY = '*';
private static final char DIVIDE = '/';
private static final char OPENBRACE = '(';
private static final char CLOSEBRACE = ')';

private InfixToPostfix() {
}

private static boolean isOperator(char c) {
return c == PLUS || c == MINUS || c == MULTIPLY || c == DIVIDE || c == OPENBRACE || c == CLOSEBRACE;
}

/**
* If op1 has a higher precedence than op2.
*
* @param op1 the first operator
* @param op2 the second operator
* @return true if op1 has a higher precendence.
*/
private static boolean higherPrecedence(char op1, char op2) {
switch (op1) {
case PLUS:
case MINUS:
return op2 == OPENBRACE;

case MULTIPLY:
case DIVIDE:
return op2 == PLUS || op2 == MINUS || op2 == OPENBRACE;

case OPENBRACE:
return true;

case CLOSEBRACE:
return false;

default: throw new IllegalArgumentException("The operator op1: " + op1 + " or operator op2: " + op2 + " is illegal.");
}
}

private static StringBuilder getOperators(Stack operatorStack) {
final StringBuilder operators = new StringBuilder();
/ eg: a + (b c), here a,b and +, (, were in their respective stacks, and current char was ) */
while (!operatorStack.isEmpty() && operatorStack.peek().charAt(0) != OPENBRACE) {
operators.append(operatorStack.pop());
}
/ pop of the remaining open brace. /
if (!operatorStack.isEmpty() && operatorStack.peek().charAt(0) == OPENBRACE) {
operatorStack.pop();
}
r

Solution

Method comments vs. naming

When the method and parameters are well names, in 9 out of 10 cases, the comment above the method is redundant. For example, higherPrecedence(char op1, char op2) - if you call op1 firstOperator and op2 secondOperator the comment above the method simply says the same thing as the method signature. Twice.

Be consistent and precise

higherPrecedence("+", "-") == false, but higherPrecendence("-", "+") == false as well... this is a very surprising result, and might result in inconsistent results.

When you get to the default case you throw an Exception which states that either op1 or op2 are illegal, although you know that op1 is the illegal one, since you don't even check if op2 is an operator.

Magic Constants

I find that "+" is much more descriptive than PLUS. These kind of constants add unnecessary ambiguity to your code, as they force the reader to go and check their actual value.

If you do decide to use a constant, I would suggest a single constant OPERATORS_BY_PRECEDENCE, which will result in these methods:

private static final OPERATORS_BY_PRECEDENCE = "(+-/*)";

private static boolean isOperator(char c) {
    return OPERATORS_BY_PRECEDENCE.indexOf(c) != -1;
}

private static boolean higherPrecendence(char firstOperator, char secondOperator) {
    return OPERATORS_BY_PRECEDENCE.indexOf(firstOperator) < 
        OPERATORS_BY_PRECEDENCE.indexOf(secondOperator);
}

Code Snippets

private static final OPERATORS_BY_PRECEDENCE = "(+-/*)";

private static boolean isOperator(char c) {
    return OPERATORS_BY_PRECEDENCE.indexOf(c) != -1;
}

private static boolean higherPrecendence(char firstOperator, char secondOperator) {
    return OPERATORS_BY_PRECEDENCE.indexOf(firstOperator) < 
        OPERATORS_BY_PRECEDENCE.indexOf(secondOperator);
}

Context

StackExchange Code Review Q#46486, answer score: 4

Revisions (0)

No revisions yet.