HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Another stack implementation (in C)

Submitted by: @import:stackexchange-codereview··
0
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:

#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 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 use
this 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 #defined
limit

#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 10
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
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 perror
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.

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(&current);
}
#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.