patterncModerate
Very simple stack in C
Viewed 0 times
simplestackvery
Problem
I'm starting to learn C and thought it would be a good idea to try and implement some data structures. I started with a very simple stack and would like to hear some opinions.
Stack.c
Stack.h
Stack.c
#include
#include
#include "stack.h"
struct Node {
int element;
struct Node* next;
};
struct Node* head;
int size;
int stack_init() {
struct Node* dummy = malloc(sizeof(struct Node));
if(dummy == NULL) {
return -1;
}
head = dummy;
return 0;
}
int push(int value) {
struct Node* tmp = malloc(sizeof(struct Node));
if (tmp == NULL) {
return -1;
}
tmp->element = value;
tmp->next = head;
head = tmp;
size++;
return 0;
}
int pop() {
if(size == 0) {
printf("Stack is empty\n");
}else {
printf("Element #%d ->value: %d has been removed\n", size, head->element);
free(head);
head = head->next;
size--;
}
return 0;
}
void print_stack() {
struct Node* tmp = head;
printf("Stacksize: %d\n", size);
for(int i = 0;i %d\n",i,tmp->element);
tmp = tmp->next;
}
printf("#####################\n");
}
int get_size() {
return size;
}Stack.h
#ifndef STACK_H
#define STACK_H
int stack_init();
int push(int value);
int pop();
void print_stack();
#endifSolution
I see a number of things that may help you improve your code.
Use a stack pointer as a parameter
Right now, one can only have one stack at a time. Worse, if one calls
Initialize all values before use
In the
Provide a way to free memory
Right now, the only way to free memory is to repeatedly call
Provide a complete interface
At the moment, there isn't any way to actually use the stack except to call
Don't access freed memory
These lines have a problem:
The problem is that after you've freed
Separate printing from data manipulation
There is no way at the moment to use
Return something useful from functions
Most of your non-void functions return something useful, but
Encapsulate related data in a structure
The
Use a typedef to simplify your code
If you use a
the code can refer to simply
Use a stack pointer as a parameter
Right now, one can only have one stack at a time. Worse, if one calls
stack_init after some items have already been pushed onto the stack, there will be a memory leak. An alternative scheme would be to have stack_init() return a pointer to a Stack and then use that as a parameter for all other calls. That way, one could have multiple stacks simultaneously, making it that much more useful.Initialize all values before use
In the
stack_init routine, the head variable is initialized, but neither head->element nor head->next are initialized. It's also probably a good idea to explicitly zero the size. Although it's already zeroed at the moment because it's a file scope variable, if you follow the other advice on this list, you'll have to initialize it yourself.Provide a way to free memory
Right now, the only way to free memory is to repeatedly call
pop(). Unfortunately, the calling program has no way to know how many items are on the stack or to know when the stack is empty, so it's not going to be able to know how many times to do so. I'd suggest providing at least an explicit isEmpty() call.Provide a complete interface
At the moment, there isn't any way to actually use the stack except to call
print_stack() which limits its usefulness in the extreme. I'd suggest that adding a means of examining the top of the stack, as with a call to top() that returns the value for that item might be a good idea.Don't access freed memory
These lines have a problem:
free(head);
head = head->next;The problem is that after you've freed
head, you dereference it to get the next pointer. This is undefined behavior -- anything could happen and it probably won't be good! Better would be to save a copy of the pointer, do your housekeeping and then free the copy like this:struct Node* temp = head;
head = head->next;
free(temp);Separate printing from data manipulation
There is no way at the moment to use
pop without it printing something. In a real program that use such a stack, that's unlikely to be the desired effect. Better would be to have pop() just do what it needs to do and leave printing to the calling program.Return something useful from functions
Most of your non-void functions return something useful, but
pop always returns 0 which isn't very useful.Encapsulate related data in a structure
The
size and head elements are closely related items. For that reason (and to accomodate suggestion #1 on this list), I'd recommend combining them into a struct instead like this:struct Stack {
struct Node* head;
int size;
};Use a typedef to simplify your code
If you use a
typedef like this:typedef struct {
struct Node* head;
int size;
} Stack;the code can refer to simply
Stack instead of struct Stack. It's not necessary, but it does tend to make things a little easier to write and to read.Code Snippets
free(head);
head = head->next;struct Node* temp = head;
head = head->next;
free(temp);struct Stack {
struct Node* head;
int size;
};typedef struct {
struct Node* head;
int size;
} Stack;Context
StackExchange Code Review Q#144966, answer score: 14
Revisions (0)
No revisions yet.