0
\$\begingroup\$

Please review my balanced parenthesis string checker code below.

I would love to know how to improve my algorithm and if you have any other feedback.

// comments inline

function validateParenthesisString(inputString){

  if(inputString.length === 0){ // empty string is as good as a valid string
    console.log('String is valid.');
    return;
  }

  var element,
      parenthesisStack = [];

	// iterate through the string
  for(var index = 0, length = inputString.length; index < length; index++){

    element = inputString.charAt(index); // cache the character

    switch(element){
      case '(':
        parenthesisStack.push('('); // openeing bracket, so just push it
        break;

      case ')':
        if(parenthesisStack[parenthesisStack.length-1] === '('){ // check if corresponding counterpart pushed already
          parenthesisStack.pop(); // remove
        } else {
          parenthesisStack.push(')');
        }
        break;

      case '[':
        parenthesisStack.push('[');
        break;

      case ']':
        if(parenthesisStack[parenthesisStack.length-1] === '['){
          parenthesisStack.pop();
        } else {
          parenthesisStack.push(']');
        }
        break;

      case '{':
        parenthesisStack.push('{');
        break;

      case '}':
        if(parenthesisStack[parenthesisStack.length-1] === '{'){
          parenthesisStack.pop();
        } else {
          parenthesisStack.push('}');
        }
        break;

      default: // garbage character
        console.log('Invalid character encountered: ' + inputString.charAt(index));
        console.log('Please enter a valid string.');
        console.log('parenthesisStack:', parenthesisStack);
        console.log('***********************************');
        return;
    };

  }

  console.log('parenthesisStack:', parenthesisStack); // just for debugging

	// check parenthesisStack
  if(parenthesisStack.length === 0){
    console.log('String is valid!');
  } else {
    console.log('String is invalid!');
  }

  console.log('***********************************');

};

validateParenthesisString('test'); // invalid
validateParenthesisString('({)}[()]{'); // invalid
validateParenthesisString('[({})]{}'); // valid

Equivalent jsFiddle.

\$\endgroup\$
1
\$\begingroup\$

Major

Overall, it seems good.

I would not check for a base case as your code can deal with empty strings. It adds complexity and clutter.

Additionally,

if(parenthesisStack[parenthesisStack.length-1] === '['){
    parenthesisStack.pop();
} else {
    parenthesisStack.push(']');
}

is repeated for each closing option. This violates the Don't Repeat Yourself principle strongly. Encapsulating it into a function would be a great idea.

The same goes to the opening values, they are always handled the same way, why not check if the input is one of them and give that the same treatment. You would only simplify code and ease future maintenance this way. Be aware that I am not really a fan of switches anywhere, except for enumerated types. But my advice is unbiased, as far as I feel it. You'd be better without a switch here, in my opinion.

Minor

You have a // remove comment. It is quite pointless but it is too small to be an issue.

Lastly, it seems that the inline comment before the loop is misaligned. It would be nice to fix that.

Algorithm-wise

Your algorithm only fails on the end. So if I start with } and follow it by () repeated a million times it will take a reasonable amount of time to terminate even though the string cannot possibly be valid.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I have fixed it here: jsfiddle.net/rdesai/zq1Lnhkj/4 \$\endgroup\$ – Rahul Desai Apr 4 '16 at 3:12
  • 1
    \$\begingroup\$ @RahulDesai Nothing to fix. Just space for improvement. It is better already. Do you see how it could be made shorter? With so many people behind crappy mobile connections, having smaller JS files is great (this is just another reason for avoiding redundant code, readability is still the main one). \$\endgroup\$ – Bernardo Sulzbach Apr 4 '16 at 3:44

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.