3
\$\begingroup\$

The function belows validates if a mathematical expression, say 12 + 2 * (9 - 3) / 4, does not have operators that are next to each other. It returns false if an operator is next to another operator. Otherwise, it returns true. For example, if the input expression is 2++1, it will return false. The function also recursively evaluates expressions that are inside pairs of parentheses.

private boolean validateOperators(String expression)
{
    if (!validateEndCharacters(expression)) {
        return false;
    }

    Stack s = new Stack(15); // Stack is a user-defined class.
    char[] operators = { '+', '-', '*', '/' };
    char prevOperator = '\0';
    String subexpression = "";
    boolean addSubexpression = false;
    boolean isValid = false;

    for (char c : expression.toCharArray()) {
        if (addSubexpression) {
            if (c == '(') {
                s.push("(");
            } else if (c == ')') {
                s.pop();
                if (s.isEmpty()) {
                    // We found the end of the expression inside the parentheses.
                    isValid &= this.validateOperators(subexpression);
                    subexpression = "";
                    prevOperator = '\0';
                    addSubexpression = false;
                }
            }

            subexpression += Character.toString(c);
        } else {
            if (c == '(') {
                addSubexpression = true;
                s.push("(");
            } else if (this.isIn(c, operators) && prevOperator == '\0') {
                prevOperator = c;
            } else if (this.isIn(c, operators) && prevOperator == '\0') {
                isValid &= false;
                break;
            } else if (Character.isDigit(c)) {
                prevOperator = '\0';
            }
        }
    }

    if (!s.isEmpty()) {
        return false;
    }

    return isValid;
}

private boolean validateEndCharacters(String expression)
{
    char[] operators = { '+', '-', '*', '/' };
    if (this.isIn(expression.charAt(0), operators) ||
       this.isIn(expression.charAt(expression.length() - 1), operators)) {
        return false;
    }

    return true;
}

How else can I improve the functions?

\$\endgroup\$
8
  • 1
    \$\begingroup\$ You make no use of strings poped from the stack. Actually, you just count left and right parentheses by a depth of the growing and shrinking stack, and test if it is empty or not. So it might be enough to replace the stack with a simple integer counter of the nesting depth. It would be incremented on '(', decremented on ')' and tested for 0 to check a subexpression, and for (-1) to verify parens balance. \$\endgroup\$ Commented Mar 30, 2017 at 11:10
  • \$\begingroup\$ @CiaPan So if the parentheses are balanced, then the integer counter would equal to 0. Correct? \$\endgroup\$ Commented Mar 30, 2017 at 13:10
  • \$\begingroup\$ It looks like this code never returns true. isValid starts as false and, as far as I can see, is never set to true. \$\endgroup\$ Commented Mar 30, 2017 at 13:52
  • 1
    \$\begingroup\$ Please don't update the code when you receive answers. If you have another question about the new code, post a new question. \$\endgroup\$ Commented Mar 30, 2017 at 14:53
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ Commented Mar 30, 2017 at 14:57

1 Answer 1

Reset to default
1
\$\begingroup\$

You can already reuse operators in each methods by making it a constant. You can create a Set and use the containsmethod instead of this.isIn.

You can simplify validateEndCharacters, if ( condition ) { return true } else { return false } can always be simplified by return condition.

The two else if looks like the second block will never be executed :

...
} else if (this.isIn(c, operators) && prevOperator == '\0') {
    prevOperator = c;
} else if (this.isIn(c, operators) && prevOperator == '\0') {
    isValid &= false;
    break; 
}
\$\endgroup\$
1
  • \$\begingroup\$ +1 on suggesting on using Sets. I forgot to use them in the function. Good catch! \$\endgroup\$ Commented Mar 30, 2017 at 12:10

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.