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

Int stack implementation

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

-
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 error


Memory 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 error

Context

StackExchange Code Review Q#42834, answer score: 12

Revisions (0)

No revisions yet.