Simple Stack Implementation in C

Question

This is my implementation of the stack data type in C language. It is static meaning it does not grow in size. Any suggestions or feedback would be much appreciated.

#include <stdio.h>
#define MAXSTACKLENGTH 6
#define true 1
#define false 0
struct stk{
 char content[MAXSTACKLENGTH];
 int top;
};
typedef struct stk stack;
int size(stack *s)
{
 return s->top;
}
int isEmpty(stack *s)
{
 if (size(s) <= -1)
 {
 return true;
 }
 else if (size(s) > -1)
 {
 return false;
 }
}
int isFull(stack *s) {
 if (size(s) == (MAXSTACKLENGTH-1))
 {
 return true;
 }
 else {
 return false;
 }
}
char peek(stack *s)
{
 if (isEmpty(s) == true)
 {
 printf("Can't peek: Stack is empty!\n");
 return '0円';
 }
 else {
 return s->content[s->top];
 }
}
void push(stack *s, char element)
{
 if (isFull(s) == true)
 {
 printf("Cant push: Stack is full!\n");
 }
 else {
 s->top++;
 s->content[s->top] = element;
 }
}
void pop(stack *s)
{
 if (isEmpty(s) == true) {
 printf("Cant pop: Stack is empty!\n");
 }
 else {
 s->top--;
 }
}
void input_push(stack *s)
{
 char c;
 printf("Enter a character> ");
 scanf(" %c", &c);
 push(s, c);
}
void print_stack(stack *s)
{
 if (isEmpty(s)) {
 printf("Stack is empty!\n");
 }
 else {
 int i = s->top;
 for (i; i > -1; i--)
 {
 printf("%c\n", s->content[i]);
 }
 printf("\n");
 }
}
void pause()
{
 char c;
 printf("Enter any character to continue.\n");
 scanf(" %c", &c);
}
int main()
{
 printf("SimpleStack! v1\n");
 stack mystack;
 mystack.top = -1; //Empty
 int run = true;
 int choice = 0;
 while (run)
 {
 if (isFull(&mystack))
 {
 printf("Warning -> Stack is full\n");
 }
 if (isEmpty(&mystack))
 {
 printf("Warning -> Stack is empty\n");
 }
 printf("1 -> Push\n");
 printf("2 -> Pop\n");
 printf("3 -> Peek\n");
 printf("4 -> Print Stack\n");
 printf("5 -> Exit\n");
 printf("Enter choice> ");
 scanf("%d", &choice);
 switch (choice) {
 case 1:
 input_push(&mystack);
 break;
 case 2:
 pop(&mystack);
 break;
 case 3:
 printf("%c\n", peek(&mystack));
 pause();
 break;
 case 4:
 print_stack(&mystack);
 pause();
 break;
 case 5:
 printf("Goodbye!\n");
 run = false;
 break;
 default:
 printf("Invalid choice.\n");
 break;
 }
 system("clear");
 }
 return 0;
}

Answer 1

It looks good. Here are a few things you can add to it or change:

• You don't have to define true/false yourself. It's unfortunate that bool is not a built-in type in C, but the Standard C Library at least provides an #include file that gives you those definitions in a portable way. So you can #include <stdbool.h> and it will define the type bool and true/false constants.

• You can combine the structure declaration + typedef into the same declaration:

  typedef struct stk 
  {
   char content[MAXSTACKLENGTH];
   int top;
  } stack;
  

  Most C programs you'll find use this more compact notation of typedef struct.

• You can simplify statements like this and others:

  int isEmpty(stack *s)
  {
   if (size(s) <= -1)
   {
   return true;
   }
   else if (size(s) > -1)
   {
   return false;
   }
  }
  

  By simply returning the condition:

  int isEmpty(stack *s)
  {
   return size(s) <= -1;
  }
  

  The result of a comparison is always a boolean, so it can be returned directly if the function also returns true/false. This is another pattern that you will see used a lot. It makes code more concise.

• Comparing against true/false in a conditional is not very usual:

  if (isFull(s) == true)
  

  You gave good descriptive names to your functions, so you can just test them like this:

  if (isFull(s)) { ...
  

  And to test if not full, the more usual way is to apply the ! (logical not) operator:

  if (!isFull(s)) { ... // Reads "if not isFull"
  

• size() currently return -1 for an empty stack. That's unusual. It doesn't make sense for a size to be negative. I'd expect an empty stack to have size 0. You should change that.

• I suggest having a init_stack() function that sets the expected initial invariants when a new stack is declared. Better than having to remember to manually set top to -1, for example.

• Since you already have a custom pause() function, you don't have to use system("pause"). The latter is non-portable and not recommended for use in real-life programs due to security risk it might incur. Best if you avoid it.

• A minor nitpick, you're mixing different styles of curly brace positioning. Some are on the same line as the parent conditional, some are on their own line. This creates a break in your style, which is a little annoying. It would be nice if you kept them consistent, independent of which style you decide for.

• penguinguy: I know the curly brace positioning annoys me too. I switch between Java and C so much sometimes I forget their styling guidelines, so that ends up happening.
• glampert: @penguinguy, I use Clang-Format in my editor. Makes life a lot easier ;)

Answer 2

In addition to adding a init_stack method as @glampert suggested to sanitize the initial content, you should reset the content after popping.

The reason is safety. If you don't initialize/reset the content, and later you access it illegally by mistake, you may find some values there that make the stack appear to be working, which can be misleading and lead to errors.

It's also unusual that the pop method doesn't return the element.

Based on the above succession, I suggest to rewrite like this:

char pop(stack *s)
{
 char item;
 if (isEmpty(s)) {
 printf("Cant pop: Stack is empty!\n");
 item = '0円';
 }
 else {
 item = s->content[s->top];
 s->content[s->top] = '0円';
 s->top--;
 }
 return item;
}

}

Answer 3

Here are some things that you might be able to use to improve your program.

Declare loop variable within for loop

The code currently contains these curious lines of code:

int i = s->top;
for (i; i > -1; i--)

The first clause in the for loop has no effect. The more idiomatic C way to write that would be this:

for (int i = s->top; i > -1; i--)

Make sure you have all required #includes

The code uses system but doesn't #include <stdlib.h>. Better yet, see the following suggestion.

Don't use system("clear")

There are two reasons not to use system("clear"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named clear, your program may execute that program instead of what you intend, and that other program could be anything. First, isolate into a seperate function clear() and then modify your code to call that function instead of system. Then rewrite the contents of that function to do what you want using C. For example, if your terminal supports ANSI Escape sequences, you could use this:

void clear()
{
 printf("\x1b[2J");
}

Ensure every control path returns a proper value

The isEmpty routine returns false or true but both are inside if clauses, leading the compiler (and the human reader of your code) wondering what gets returned if both if clauses are false. Fortunately, it's easy to rewrite this to take care of that:

int isEmpty(stack *s)
{
 return size(s) <= -1;
}

Use const where practical

The current isEmpty() routine does not (and should not) modify the passed stack, and so it should be declared const:

int isEmpty(const stack *s)

The same is true for all of the other functions which don't alter the stack.

Consider separating input and output

Right now, the peek, push and pop functions each print something if the stack is not in a state that would allow the operation. This limits reusability. Instead, consider modifying each routine to return a value signalling failure. For example:

int peek(const stack *s)
{
 if (isEmpty(s)) {
 return EOF;
 }
 return s->content[s->top];
}

This way, the calling routine can decide how to handle I/O, making it much easier to reuse this code.

Combine typedef and struct declaration

You can combine the typedef with the struct declaration, making the code more compact and making it even simpler for a human reader of your code to understand the typedef:

typedef struct stk{
 char content[MAXSTACKLENGTH];
 int top;
} stack;

Eliminate magic numbers

You've done well to define MAXSTACKLENGTH, but the constant -1 is used to signify an empty stack, so it should probably also have a #define.

Give special structures their own initializer

The stack is properly initialized with a value of -1 as mentioned above. Rather that having that in the main routine, it would be better if there were a separate stack_init() function that would take care of any initialization required. It's not much difference here, but as your structures get more complex, this practice tends to make them much easier to use consistently and correctly.

Use stdbool.h instead of your own boolean values

Instead of #defineing your own true and false values, it's better to #include <stdbool.h> which will give you both of those values as well as a a bool type that you can use.

Eliminate return 0 at the end of main

Since C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.

• penguinguy: I'm confused with declaring the index variable inside the for loop because every time I tried to do that it gives a compiler error.
• Edward: Either you're using a very old compiler or you're trying to use the index variable after the loop. When the index variable is declared inside the loop, it's only in scope inside the loop and the compiler should report an error in that case.