patterncMinor
Another stack implementation (in C)
Viewed 0 times
implementationanotherstack
Problem
I just started learning C and the online book contained the exercise 'implement a stack'. So I did, but thought I'd put it here, because I still don't feel comfortable with pointers.
So here it is:
Is there anything wrong/not good (I mean two different things here) with this?
So here it is:
#include
#include
struct StackItem {
struct StackItem *previous;
int value;
};
void stack_push(struct StackItem **current, int value) {
struct StackItem *tmp = malloc(sizeof(struct StackItem));
tmp->previous = *current;
tmp->value = value;
*current = tmp;
}
void stack_pop(struct StackItem **current) {
if ((*current)->previous != NULL) {
struct StackItem *tmp = (*current)->previous;
free(*current);
*current = tmp;
} else printf("%s\n", "Previous is NULL");
}
void stack_print(struct StackItem *current) {
printf("%d\n", current->value);
while (current->previous != NULL) {
current = current->previous;
printf("%d\n", current->value);
}
}
int main(int argc, char **argv) {
struct StackItem *current = malloc(sizeof(struct StackItem));
current->value = 2147483647;
current->previous = NULL;
char input[10];
int i = 0;
while (i < 10) {
puts("Enter integer value to add to the stack:");
gets(input);
char *end;
int newVal = (int)strtol(input, &end, 10);
if (*end != '\0') { puts("That was no integer value."); --i; }
else stack_push(¤t, newVal);
++i;
}
stack_print(current);
puts("Stack printed once.");
i = 0;
while (i < 10) {
stack_pop(¤t);
i++;
}
stack_print(current);
puts("Stack printed twice.");
return 0;
}Is there anything wrong/not good (I mean two different things here) with this?
Solution
Your code looks nice and compiles cleanly - good signs. Some comments, nevertheless:
-
all functions except
-
the opening brace { for a function should be in column 0
-
-
the stack variable name
-
you define the stack
this item. It would be better to define the stack using just a pointer:
this involves some changes to other functions because the 'empty stack'
condition is no longer
-
for-loops are often better than while loops - eg when you are counting
through a range. And the loop variable is best declared in the loop where
possible:
-
using constants in the code like 10 is often a bad idea. Some are fine (eg
the 10 in the
limit
Such #defines go at the top of the file after the #includes
-
bytes, if the user types in more than 10 characters the buffer overflows and
overwrites the call stack (not your stack but the process stack). Use
-
there is no error handling, but that is ok for an exercise. In a real
program, you would want to check that
to print an error message if it does - an then you have to decide what to do
about the failure) and you might want to return success or failure from
the push/pop functions.
-
all functions except
main should be declared static-
the opening brace { for a function should be in column 0
-
stack_print should take a const parameter, as you are not modifying the stack:void stack_print(const struct StackItem *current)-
the stack variable name
current would be better as (for example) stack-
you define the stack
current by allocating a StackItem but never usethis item. It would be better to define the stack using just a pointer:
struct StackItem *current = NULL;this involves some changes to other functions because the 'empty stack'
condition is no longer
(current)->previous == NULL but simply current
== NULL. This affects stack_pop and stack_print. The latter becomes:void stack_print(const struct StackItem *current)
{
while (current != NULL) {
printf("%d\n", current->value);
current = current->previous;
}
}-
for-loops are often better than while loops - eg when you are counting
through a range. And the loop variable is best declared in the loop where
possible:
for (int i = 0; i < 10; ++i) {
stack_pop(¤t);
}-
using constants in the code like 10 is often a bad idea. Some are fine (eg
the 10 in the
strtol call, but the loops would be better having a #definedlimit
#define N_VALUES 10
for (int i = 0; i < N_VALUES; ++i) {...}Such #defines go at the top of the file after the #includes
-
gets is not safe and should not be used. Using gets with a buffer of 10bytes, if the user types in more than 10 characters the buffer overflows and
overwrites the call stack (not your stack but the process stack). Use
fgets instead:fgets(input, (int) sizeof input, stdin);-
there is no error handling, but that is ok for an exercise. In a real
program, you would want to check that
malloc does not fail (using perrorto print an error message if it does - an then you have to decide what to do
about the failure) and you might want to return success or failure from
the push/pop functions.
Code Snippets
void stack_print(const struct StackItem *current)struct StackItem *current = NULL;void stack_print(const struct StackItem *current)
{
while (current != NULL) {
printf("%d\n", current->value);
current = current->previous;
}
}for (int i = 0; i < 10; ++i) {
stack_pop(¤t);
}#define N_VALUES 10
for (int i = 0; i < N_VALUES; ++i) {...}Context
StackExchange Code Review Q#25374, answer score: 4
Revisions (0)
No revisions yet.