\$\begingroup\$
\$\endgroup\$
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);
}
1 Answer 1
\$\begingroup\$
\$\endgroup\$
1
Various thoughts
- You never use the
max_size
field, so you should delete it. - When pushing, you never check to see if the stack is full, so you could overrun your array if you push too many items.
- Your current implememtation does not utilize the first array slot
items[0]
. You could make a few easy changes to fix that. - You have a check for
position < 0
but it can never go negative. You could even makeposition
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
-
\$\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\$Isty001– Isty0012016年02月07日 15:47:50 +00:00Commented Feb 7, 2016 at 15:47
lang-c