patterncModerate
Int stack implementation
Viewed 0 times
implementationintstack
Problem
#include
#include
struct Stack {
int value;
int is_empty;
struct Stack *next;
};
int pop(struct Stack **stack) {
if ((*stack)->is_empty == 1) {
printf("Stack is empty!\n");
abort(); // not sure what this does
} else {
int value = (*stack)->value;
// outer pointer (static) points to new stack now
*stack = (*stack)->next;
return value;
}
}
void push(struct Stack **stack, int value) {
// malloc is used so this memory can be accessed outside of scope
struct Stack *head = malloc(sizeof(struct Stack*));
if (head != NULL) {
head->value = value;
// head's next is pointer to stack
head->next = *stack;
head->is_empty = 0;
// stack now holds pointer to head
*stack = head;
} else {
printf("push memory no alloc");
}
}
struct Stack *empty() {
struct Stack *head = malloc(sizeof(struct Stack *));
if (head != NULL) {
head->value = 0;
head->is_empty = 1;
head->next = NULL;
return head;
} else {
return NULL;
}
}
int main() {
struct Stack *init = empty();
struct Stack **stack = &init;
push(stack, 3);
push(stack, 2);
push(stack, 1);
int one = pop(stack);
printf("one: %d\n", one);
int two = pop(stack);
printf("two: %d\n", two);
int three = pop(stack);
printf("three: %d\n", three);
//int hmm = pop(stack);
}I'm not sure pointers to pointers is the best way to do this. I also feel like I'm writing code in a Java-y way, and would like to know how to improve my C style.
Also, are there any edge cases I am missing?
Solution
Things you did well:
-
Overall the code looks very nice and well organized.
-
You used comments well.
Things you could improve:
Syntax:
-
The
-
You can simplify your
Since
-
If you don't take in any variables as parameters, you should declare them as
Error handling:
-
Don't use
Memory usage:
-
You never
You should probably
Returning:
-
You don't
-
Overall the code looks very nice and well organized.
-
You used comments well.
Things you could improve:
Syntax:
-
typedef your structs.struct Stack {
int value;
int is_empty;
struct Stack *next;
};The
typedef means you no longer have to write struct all over the place. That not only saves some space, it also can make the code cleaner since it provides a bit more abstraction.typedef struct
{
int value;
int is_empty;
struct Stack *next;
} Stack;-
You can simplify your
NULL checks.if (head != NULL)Since
NULL is defined as (void *)0, we can treat is as a comparison to 0, making the != NULL a redundant check.if (head)-
If you don't take in any variables as parameters, you should declare them as
void.int main(void)Error handling:
-
Don't use
abort(). The function raises the SIGABRT signal. This, if uncaught, causes the program to terminate returning a platform-dependent unsuccessful termination error code to the host environment. The program is terminated without destroying any object and without calling any of the functions passed to atexit or at_quick_exit. It would be much better to return an error indicator and have the calling function handle it.return -1; // we have an errorMemory usage:
-
You never
free() your init value.struct Stack *init = empty();
// within empty()
struct Stack *head = malloc(sizeof(struct Stack *));
return *head;You should probably
free() it before your program terminates. Failure to deallocate memory using free() leads to buildup of non-reusable memory, which is no longer used by the program. This wastes memory resources and can lead to allocation failures when these resources are exhausted.free(init);Returning:
-
You don't
return a value from main(), even though you state you do.int main() {
// no return value... no indication of successful exit
}Returning 0 is usually a typical indication of a successful exit.return 0;Code Snippets
struct Stack {
int value;
int is_empty;
struct Stack *next;
};typedef struct
{
int value;
int is_empty;
struct Stack *next;
} Stack;if (head != NULL)int main(void)return -1; // we have an errorContext
StackExchange Code Review Q#42834, answer score: 12
Revisions (0)
No revisions yet.