semantical error with parenthesis balancing here

Question

I've written a C code to check for simple parenthesis balancing which prints NO and YES if balanced, not balanced respectively.

The problem is that I'm getting NO for all inputs. Hence I'm thinking that its a semantic error but cannot find it (I have been trying for 2 days :p)

Could someone please help me here? thanks!

#include <stdio.h>
#include <stdlib.h>

typedef struct stack {
    char s[1000];
    int top;
} STACK;

void create(STACK *sptr) {
    sptr->top = -1;
}

int isEmpty(STACK *sptr) {
    if (sptr->top == -1)
        return 1;
    else
        return 0;
}

void push(STACK *sptr, char data) {
    sptr->s[++sptr->top] = data;
}

char pop(STACK *sptr) {
    if (isEmpty(sptr) == 0)
        return sptr->s[sptr -> top];
    else
        return '$';
}

char *isBalanced(char *s, STACK *sptr) {
    char *y, *n;
    int i = 0;
    y = (char*)malloc(sizeof(char) * 4);
    y[0] = 'Y';
    y[1] = 'E';
    y[2] = 'S';
    y[3] = '\0';
    n = (char*)malloc(sizeof(char) * 3);
    n[0] = 'N';
    n[1] = 'O';
    n[2] = '\0';

    while (s[i] != '\0') {
        if (s[i] == '(' || s[i] == '{' || s[i] == '[') {
            push(sptr, s[i]);
        } else {
            char x = pop(sptr);
            switch (s[i]) {
              case ')':
                if (x != '(') 
                    return n;
                break;

              case '}':
                if (x != '{') 
                    return n;
                break;

              case ']':
                if (x != '[') 
                    return n;
                break;
            }
        }
        ++i;
    }

    if (isEmpty(sptr))
        return y;
    else
        return n;
}

int main() {
    STACK *sptr = (STACK *)malloc(sizeof(STACK));
    char c[21];
    int ch;
    do {
        printf("enter sequence:");
        scanf("%s", c);
        char *msg = isBalanced(c, sptr);
        printf("%s", msg);
        printf("choice?:");
        scanf("%d", &ch);
    } while(ch);
}

updated code:

#include <stdio.h>
#include <stdlib.h>

typedef struct stack {
    char s[1000];
    int top;
} STACK;

void create(STACK *sptr) {
    sptr->top = -1;
}

int isEmpty(STACK *sptr) {
    if (sptr->top == -1)
        return 1;
    else 
        return 0;
}

void push(STACK *sptr, char data) {
    sptr->s[++sptr->top] = data;
}

char pop(STACK *sptr) {
    if (isEmpty(sptr) == 0)
        return sptr->s[sptr->top--];
    else
        return '$';
}

int isBalanced(char *s, STACK *sptr) {
    int i = 0;

    while (s[i] != '\0') {
        if (s[i] == '(' || s[i] == '{' || s[i] == '[') {
            push(sptr, s[i]);
        } else {
            char x = pop(sptr);
            switch (s[i]) {
              case ')':
                if (x != '(')
                    return 0; 
                break;
              case '}':
                if (x != '{') 
                    return 0;
                break;
              case ']':
                if (x != '[') 
                    return 0;
                break;
            }
        }
        ++i;
    }

    if (isEmpty(sptr))
        return 1;
    else 
        return 0;
}

int main() {
    STACK *sptr = malloc(sizeof(STACK));
    char c[21];
    int ch;
    do {
        printf("enter sequence:");
        scanf("%s", c);
        printf("%s", (isBalanced(c, sptr) ? "YES" : "NO"));
        printf("choice?:");
        scanf("%d", &ch);

    } while(ch);
}

Did you debug your code to observe values of relevant variables, including the return value of scanf(), while processing input? — Yunnosch, Dec 25 '18 at 15:54
You are ignoring the return value of scanf() at your own risk. Consider not to. — Yunnosch, Dec 25 '18 at 15:54
Offtopic: I'd prefer using 'size' instead of 'top', that's the more common idiom in C or C++ world (so empty stack gets 0). Then, as negative values are meaningless, I'd prefer unsigned int or possibly even size_t. — Aconcagua, Dec 25 '18 at 15:56
OT again: Comparisons in C evaluate to 1 (true)/0 (false) anyway, so you might/should prefer `return sptr -> top == -1;` (or size == 0, if you follow my previous recommendation). — Aconcagua, Dec 25 '18 at 15:58
Please learn about indentation, it will help you find more amenable minded potential answerers. — Yunnosch, Dec 25 '18 at 15:58
You `pop` with every character that's not an opening parenthesis of any type - this will fail if there are yet characters other than closing parentheses... — Aconcagua, Dec 25 '18 at 16:04
Note that the dot `.` and arrow `->` operators bind very tightly; you should not put spaces around them. — Jonathan Leffler, Dec 25 '18 at 16:05
Also, `sizeof(char)` is always `1` by definition, so multiplying by `sizeof(char)` is pointless. — Arkku, Dec 25 '18 at 16:18
Furthermore, don't cast the return value of `malloc`. https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc — Arkku, Dec 25 '18 at 16:20
Instead of returning the result as a string (and a `malloc`'d one at that, which must be freed by the caller), just return a boolean and then print `isBalanced(c, sptr) ? "YES" : "NO"`. The current naming is otherwise dangerously misleading; consider `if (isBalanced(c, sptr))` – this would be true for both cases. (And on the subject of naming: the `c` and `sptr` also unnecessarily obfuscated. Why not `text` and `stack`, for example.) — Arkku, Dec 25 '18 at 16:26
I would say that the semantic error is: not using the stack to check that a *closing* char matches the last corresponding *opening* one. In other words: you get an opening char, you push it; you get a closing char, you pop and check it matches. In the end, the stack must be empty. — linuxfan says Reinstate Monica, Dec 25 '18 at 17:16
@linuxfan i did use a stack to push the opening brackets and pop and check match when closing brackets are come across. also ive checked if stack is not empty at the end as well. — dagwood, Dec 27 '18 at 04:03
@Arkku thanks, ive made the corrections in my updated code. but it still shows NO on running for all inputs. — dagwood, Dec 27 '18 at 04:05
@Yunnosch "You are ignoring the return value of scanf() at your own risk. Consider not to." what does this mean ? why would scanf return anything useful. please elaborate. — dagwood, Dec 27 '18 at 04:09
The return to `scanf` is the number of successful conversions that took place based on your *format string*. For example `scanf("%s", c);` would return `1` for a successful conversion to string placed in `c`. `scanf` can also return `EOF` or `0` (for a failed conversion). You must check `if (scanf("%s", c) != 1) { /* handler error */ }` (at minimum) to validate the input. However, `scanf` is a poor choice as it will stop reading on the first whitespace encountered. A better choice if `fgets`. That way you could at least enter `8 + (4 / a[2])` and get more than `'8'` as input. — David C. Rankin, Dec 27 '18 at 04:48
Read the documentation of scanf(). Its return value is extremely useful when debugging any non-working program which contains any scanf() call. When you are driven to ask a question here but have not bothered to analyse the return values, then the risk of being ignored by potential answereres is higher than otherwise. — Yunnosch, Dec 28 '18 at 09:29

score 1 · Answer 1 · edited Dec 25 '18 at 16:07

1

In your pop function you are not decrementing the top thus your isEmpty function always returns false.

Hence you return no always.

char* isBalanced(char* s, STACK* sptr)
{
    ....

   if(isEmpty(sptr)) return y;
   else return n;
}

The following is the correct implementation:

char pop(STACK* sptr)
{
    if(isEmpty(sptr) == 0)
        return sptr->s[sptr->top--];
    else
        return '$';
}

edited Dec 25 '18 at 16:07

Jonathan Leffler

730,956
141
904
1,278

answered Dec 25 '18 at 15:58

kiran Biradar

12,700
3
19
44

0___________ · Answer 2 · 2018-12-25T22:00:17.113

1

I would add the flags to see which one does not match

unsigned match(const char *f)
{
    int p = 0,s = 0,b = 0;
    while(*f)
    {
        switch(*f++)
        {
            case '(':
                p++;
                break;
            case ')':
                p -= !!p;
                break;

            case '[':
                s++;
                break;
            case ']':
                s -= !!s;
                break;

            case '{':
                b++;
                break;
            case '}':
                b -= !!b;
                break;
            default:
                break;
        }
    }
    return (!p) | ((!s) << 1) | ((!b) << 2);
}

Not the stack version but just for the idea. It is rather easy to add push and pop

edited Dec 25 '18 at 22:00

answered Dec 25 '18 at 16:15

0___________

60,014
4
34
74

2

So, basically, as it is, this does not work to check balanced brackets. Correct? It only ensures this `)1(` passes. – Jongware Dec 25 '18 at 16:25
@usr2564301 yes - Christmas coding, but not after the edit – 0___________ Dec 25 '18 at 22:00

score 1 · Answer 3 · answered Dec 25 '18 at 17:33

I assume that the idea is this: an opening paren is always permitted (in the analyzed text), but a closing paren must match the last opened. This is why a stack is required. Also, in the end, if the stack is not empty that means that some parens were not closed.

So, you should use the stack, but only when you encounter parens - either opening or closing.

In the function isBalanced(), the main cycle could be this:

while (s[i] != '\0') {
    if ( s[i] == '(' || s[i] == '{' || s[i] == '[' ) {
        // opening - remember it
        push(sptr, s[i]);
    } else if ( s[i] == ')' || s[i] == '}' || s[i] == ']' ) {
        // closing - it should match
        if ( ! popmatch(sptr, s[i]) )
          return n;
    }
    ++i;
}

The rest of the function is ok. Now, I modified the the pop() function: renamed to popmatch, because it should check if the argument matches the top of the stack. If it does, pop the stack and return OK. So the function would be:

char popmatch(STACK* sptr, char c) {
    // an empty stack can not match
    if (isEmpty(sptr))
      return 0;

    // not empty, can check for equality
    if ( c =! sptr->s[sptr->top] )
      return 0;
      // does not match!

    // ok, it matches so pop it and return ok
    sptr->top--;
    return 1;
}

Please note that the code mirrors pretty well the "analysis"; when the analysis is short and clear, and the code follows the analysis, the result often is short and clear too.

@lamyabhasin could you post the code you used? A debugging session will surely discover the error. — linuxfan says Reinstate Monica, Dec 27 '18 at 05:48
what type of debugger do you use ? i dont use any as of now. i have a macbook air — dagwood, Dec 27 '18 at 11:20
@lamyabhasin I don't see new code, and I don't see the popmatch() func. About debugger, I think to use gdb on a linux system, but it is not really important. — linuxfan says Reinstate Monica, Dec 27 '18 at 11:23

chqrlie · Accepted Answer · 2018-12-27T12:39:15.487

Here are some major issues in your code:

you do not properly initialize the stack with create(sptr) before use, preferably in main() where it is defined. Your code has undefined behavior. It prints NO by chance, probably because sptr->top has an initial value of 0, which makes the stack non-empty.
you should only pop from the stack when you encounter a closing delimiter ), ] or }.
you should prevent potential buffer overflow by telling scanf() the maximum number of characters to read into c: scanf("%20s", c). Furthermore you should test the return value to avoid undefined behavior at end of file.
Note also that the STACK can be made a local variable to avoid heap allocation and potential allocation failure, which would cause undefined behavior since it is not tested.

Here is a corrected version:

#include <stdio.h>

typedef struct stack {
    char s[1000];
    int top;
} STACK;

void create(STACK *sptr) {
    sptr->top = -1;
}

int isEmpty(STACK *sptr) {
    if (sptr->top == -1)
        return 1;
    else 
        return 0;
}

void push(STACK *sptr, char data) {
    sptr->s[++sptr->top] = data;
}

char pop(STACK *sptr) {
    if (isEmpty(sptr) == 0)
        return sptr->s[sptr->top--];
    else
        return '$';
}

int isBalanced(char *s, STACK *sptr) {
    int i;

    for (i = 0; s[i] != '\0'; i++) {
        switch (s[i]) {
          case '(':
          case '{':
          case '[':
            push(sptr, s[i]);
            break;
          case ')':
            if (pop(sptr) != '(')
                return 0; 
            break;
          case '}':
            if (pop(sptr) != '{') 
                return 0;
            break;
          case ']':
            if (pop(sptr) != '[') 
                return 0;
            break;
        }
    }
    return isEmpty(sptr);
}

int main() {
    STACK s, *sptr = &s;
    char c[100];
    int ch;

    do {
        printf("Enter sequence: ");
        if (scanf(" %99[^\n]", c) != 1)
            break;
        create(sptr);
        printf("%s\n", isBalanced(c, sptr) ? "YES" : "NO");
        printf("Choice? ");
        if (scanf("%d", &ch) != 1)
            break;
    } while (ch);

    return 0;
}

`if (fgets (c, sizeof c, stdin) == NULL) break;` as a bonus. A sequence without whitespace just seems less readable. — David C. Rankin, Dec 27 '18 at 04:51
@DavidC.Rankin: you are correct about white space, but using `fgets()` here and `scanf()` for the next question would cause unexpected behavior for the second iteration of this loop. I updated the answer with a different fix. — chqrlie, Dec 27 '18 at 12:42

semantical error with parenthesis balancing here

4 Answers4