-1
\$\begingroup\$

My stack uses char data and is not tested. I use it to save command in a unix pipeline for my own POSIX shell. Maybe you can find a bug or a possible improvement.

 #define _XOPEN_SOURCE 500 
 /*
 * C program to implement stack. Stack is a LIFO data structure.
 * Stack operations: PUSH(insert operation), POP(Delete operation)
 * and Display stack.
 */
#include <stdio.h>
#include "stack.h"
/*  Function to add an element to the stack */
void push (char *num)
{

    if (s.top == (MAXSIZE - 1))
    {
        printf ("Stack is Full\n");
        return;
    }
    else
    {
        printf ("Entered the element to be pushed\n");
        s.top = s.top + 1;
        s.stk[s.top] = num;
    }
    return;
}
/*  Function to delete an element from the stack */
char* pop ()
{
    char *num;
    if (s.top == - 1)
    {
        printf ("Stack is Empty\n");
        return ("-1");
    }
    else
    {
        num = s.stk[s.top];
        printf ("popped element is = %sn", s.stk[s.top]);
        s.top = s.top - 1;
    }
    return(num);
}
/*  Function to display the status of the stack */
void display ()
{
    int i;
    if (s.top == -1)
    {
        printf ("Stack is empty\n");
        return;
    }
    else
    {
        printf ("\n The status of the stack is \n");
        for (i = s.top; i >= 0; i--)
        {
            printf ("%s\n", s.stk[i]);
        }
    }
    printf ("\n");
}

stack.h

#define _XOPEN_SOURCE 500

#ifndef OPENSHELL_STACK_H
#define OPENSHELL_STACK_H
#define MAXSIZE 50
struct stack
{
    char *stk[MAXSIZE];
    int top;
};
typedef struct stack STACK;
STACK s;
char *choice;
void push (char *num);
char* pop(void);
void display(void);

#endif
\$\endgroup\$
6
+50
\$\begingroup\$

Your description of the stack you wrote is incorrect. It does not handle char data; it handles char * data. Moreover, your functions can only operate on that one defined instance of the stack (s). This is BAD! Let's look at how we can fix this:

Use Opaque Types

One issue with the way you have your structure defined in the header file is that it exposes it's internal representation to the user. That is, if the user includes your header file, they are free to change the contents of stk and top without ever using your functions. As you can probably imagine, without proper documentation on how to properly manipulate the fields (or even with proper documentation), this is very error-prone. Ideally, you'd like your users to only be able to manipulate your structure using your API. To do this, we need to do some clean up work as shown below. One disadvantage with this approach though is that you'll be forced to use heap allocation to initialize opaque structures.

Better Naming Conventions

As it is, naming your files stack.h and your data structure stack can easily cause naming collisions with other libraries as these are very common names. Use names that are more suited for your purpose (e.g. OpenShellStack). This also comes with the nice bonus that the name is self-describing and users will be more aware that this is not a general purpose stack but one used for a specific purpose.

Stack API

In order to operate on the stack, your functions need to have the stack passed into them. Pass in the stack via pointer for modifying functions and via const pointer for non-modifying functions (e.g. your display() function). Also, in most stack implementations, there are usually two distinct operations to cover what you're doing in your pop() function. First, there's the top() function which returns to the user the top element if the stack is not empty and then there's the pop() function which removes the top element off of the stack. Also, having a is_empty() convenience function and it's counter function is_full() certainly helps. You will also want to have init() and free() functions to both initialize and destroy the stack created.

Below is a possible implementation that takes into account everything mentioned above.

OpenShellStack.h

#define _XOPEN_SOURCE 500

#ifndef OPENSHELL_STACK_H
#define OPENSHELL_STACK_H
#include <stdbool.h>

/* We can use this line to define our opaque structure. */
typedef struct OpenShellStack_ OpenShellStack;

OpenShellStack *init_stack(void);
void free_stack(OpenShellStack *s);

void push_stack(OpenShellStack *s, char elem);
char top_stack(const OpenShellStack *s);
void pop_stack(OpenShellStack *s);
bool is_empty_stack(const OpenShellStack *s);
bool is_full_stack(const OpenShellStack *s);
void display_stack(const OpenShellStack *s);
#endif

OpenShellStack.c

#include "OpenShellStack.h"
#include <stdio.h>
#define MAX_STACK_SIZE (50)

/* We define the stack structure in here now!
   This stack structure handles char data, not char * data
   like yours previously did */

struct OpenShellStack_ {
    char data[MAX_STACK_SIZE];
    int top;
};

OpenShellStack *init_stack(void)
{
    OpenShellStack *s = malloc(sizeof(*s));
    if (s) {
        s->top = -1;
    }
    return s;
}

void free_stack(OpenShellStack *s)
{
    free(s);
}

void push_stack(OpenShellStack *s, char elem)
{
    if (!is_full_stack(s)){
        s->data[++s->top] = elem;
    }
}

char top_stack(const OpenShellStack *s)
{
    return is_empty_stack(s) ? '\0' : s->data[s->top];
}

void pop_stack(OpenShellStack *s)
{
    if (!is_empty_stack(s)) {
        --s->top;
    }
}

bool is_empty_stack(const OpenShellStack *s)
{
     return s->top == -1;
}

bool is_full_stack(const OpenShellStack *s)
{
     return s->top == MAX_STACK_SIZE - 1;
}

void display_stack(const OpenShellStack *s)
{
     for (int i = s->top; i != -1; --i) {
         putchar(s->data[i]);
     }
}

main.c

#include <stdlib.h>
#include <stdio.h>
#include <assert.h>
#include "OpenShellStack.h"

#define SIZEOF_ARR(arr) (sizeof(arr) / sizeof(*arr))

int main()
{
    char data[] = "abcde";
    OpenShellStack *s = init_stack();
    assert(s && "Not enough mem for stack");
    assert(is_empty_stack(s) && "Stack should be empty");

    for (int i = 0; i < SIZEOF_ARR(data); ++i) {
        push_stack(s, data[i]);
    }
    assert(!is_full_stack(s) && "Stack shouldn't be full");

    display_stack(s);

    for (int i = 0; i < SIZEOF_ARR(data); ++i) {
        printf("The top of the stack contains character %c.\n", top_stack(s));
        pop_stack(s);
    }
    assert(is_empty_stack(s) && "Stack should be empty now");

    free_stack(s);    
}
\$\endgroup\$
  • \$\begingroup\$ Very good answer. I will award you a bounty for this tomorrow. \$\endgroup\$ – Niklas Apr 18 '16 at 14:56

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.