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\$
  • 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\$ – CiaPan Mar 30 '17 at 11:10
  • \$\begingroup\$ @CiaPan So if the parentheses are balanced, then the integer counter would equal to 0. Correct? \$\endgroup\$ – Sean Francis N. Ballais Mar 30 '17 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\$ – AlanT Mar 30 '17 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\$ – Riker Mar 30 '17 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\$ – Peilonrayz Mar 30 '17 at 14:57
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\$
  • \$\begingroup\$ +1 on suggesting on using Sets. I forgot to use them in the function. Good catch! \$\endgroup\$ – Sean Francis N. Ballais Mar 30 '17 at 12:10

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.