3
\$\begingroup\$

I just started to learn C, and for practicing, I tried to implement a simple stack data structure, and I want to know if I'm making any mistakes, or how could I improve it:

My stack.h:

#ifndef STACK_H
#define STACK_H
#define STACK_MAX_SIZE 125
typedef struct
{
 int max_size;
 int items[STACK_MAX_SIZE];
 int position;
} Stack;
void stack_push(Stack *stack, int number);
int stack_pop(Stack *stack);
bool stack_is_empty(Stack *stack);
#endif

Stack.c:

#include <stdio.h>
#include <stdbool.h>
#include "stack.h"
void stack_push(Stack *stack, int number)
{
 if(stack->position < 0){
 stack->position = 1;
 }
 stack->position++;
 stack->items[stack->position] = number;
}
int stack_pop(Stack *stack)
{
 if(stack->position == 0){
 fprintf(stderr, "Stack is empty");
 return -1;
 }
 int number = stack->items[stack->position];
 stack->position--;
 return number;
}
bool stack_is_empty(Stack *stack)
{
 return stack->position < 1;
}

And the test:

#include <stdarg.h>
#include <stddef.h>
#include <setjmp.h>
#include <cmocka.h>
#include <stdio.h>
#include <stdbool.h>
#include "../src/stack.h"
static void stack_test() {
 Stack s, *stack = &s;
 for(int i = 0; i <= 5; i++){
 stack_push(stack, i);
 }
 assert_int_equal(stack_pop(stack), 5);
 while(false == stack_is_empty(stack)){
 stack_pop(stack);
 }
 assert_true(stack_is_empty(stack));
}
int main(void) {
 const struct CMUnitTest tests[] = {
 cmocka_unit_test(stack_test),
 };
 return cmocka_run_group_tests(tests, NULL, NULL);
}
asked Feb 7, 2016 at 10:08
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Various thoughts

  1. You never use the max_size field, so you should delete it.
  2. When pushing, you never check to see if the stack is full, so you could overrun your array if you push too many items.
  3. Your current implememtation does not utilize the first array slot items[0]. You could make a few easy changes to fix that.
  4. You have a check for position < 0 but it can never go negative. You could even make position an unsigned value.

Rewrite

Here are your push and pop functions rewritten:

void stack_push(Stack *stack, int number)
{
 if (stack->position >= STACK_MAX_SIZE) {
 fprintf(stderr, "Stack is full\n");
 return;
 } 
 stack->items[stack->position++] = number;
}
int stack_pop(Stack *stack)
{
 if (stack->position == 0) {
 fprintf(stderr, "Stack is empty\n");
 return -1;
 }
 return stack->items[--stack->position];
}
answered Feb 7, 2016 at 11:31
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback. I actually wanted to use that for keep increasing the size of the stack. I modified the if inside the push function with it: stack->items = realloc(stack->items, (stack->size * sizeof(int))); \$\endgroup\$ Commented Feb 7, 2016 at 15:47

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.