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

Valid Parentheses

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

Problem

Given a string containing just the characters '(', ')', '{', '}', '['
and ']', determine if the input string is valid.


The brackets must close in the correct order, "()" and "()[]{}" are
all valid but "(]" and "([)]" are not.

The following is my code, which has already been tested on a onlinejudge.
Can someone help me give some comments for the code itself?

bool isValid(string s) {
    stack s1;
    string::iterator it = s.begin();
    map x;
    x['('] = ')';
    x['['] = ']';
    x['{'] = '}';

    for (; it!=s.end(); ++it) {
        if (!s1.empty()) {
            char c = s1.top();
            if(x[c] == *it) {
                s1.pop();
            }
            else {
                s1.push(*it);
            }
        }
        else {
            s1.push(*it);
        }
    }   

    return s1.empty();
}

Solution

You use of objects without prefix them with std:: is a bad sign.

I bet you used using namespace std; in the code. Please stop this habit.

https://stackoverflow.com/q/1452721/14065

When passing objects to functions prefer to pass by const reference to prevent unnecessary copies.

bool isValid(std::string const& s) {
     //      ^^^^^       ^^^^^^


To prevent you accidentally modifying things try and use cost_itrerator when you need only read only accesses to a container:

std::string::const_iterator it = s.begin();


In C++11 you can use the new 'brace init' feature:

map x;
x['('] = ')';
x['['] = ']';
x['{'] = '}';

//replace with:
 map x = {{'(', ')'}, {'[',']'},{'{','}'}};


Also because this is a imutable you can make it const and static. The const will help prevent errors while the static is so you only create it once.

static map const x = {{'(', ')'}, {'[',']'},{'{','}'}};

Code Snippets

bool isValid(std::string const& s) {
     //      ^^^^^       ^^^^^^
std::string::const_iterator it = s.begin();
map<char, char> x;
x['('] = ')';
x['['] = ']';
x['{'] = '}';

//replace with:
 map<char, char> x = {{'(', ')'}, {'[',']'},{'{','}'}};
static map<char, char> const x = {{'(', ')'}, {'[',']'},{'{','}'}};

Context

StackExchange Code Review Q#24208, answer score: 4

Revisions (0)

No revisions yet.