ADT stack with a dynamic array (revision 1)

Question

Here's the second draft of my ADT stack code which I posted here before after carrying out most of the improvements suggested there.

I decided to expose a function called resize_stack to adjust the size of the stack as desired by the user instead of doing it automatically.

stacklib.h

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>

#ifndef STACKLIB_H
#define STACKLIB_H


/* generic type pushed item_to the stack */    
/* in this case it's just an int */
typedef int item_t;

/* our stack data type */
typedef struct {
    size_t size;
    size_t capacity;
    item_t *elements;
} stack;

/* interface to our new stack data type */
stack* create_stack(size_t capacity);
int push_stack(stack *mystack, item_t data);
item_t* pop_stack(stack *mystack);
void delete_stack(stack *mystack);
stack* resize_stack(stack *mystack, size_t capacity);

#endif

stacklib.c

#include "stacklib.h"

/* a helper function to handle exceptions */
void handle_error(const char* message)
{
    if(errno) {
        perror(message);
    } else {
        printf("Error: %s\n", message);
    }

    exit(1);
}


stack* create_stack(size_t capacity)
{
    stack *stack_ptr = malloc(sizeof(stack));

    if(stack_ptr == NULL)
        handle_error("Error while allocating memory.");

    item_t *elements_ptr = malloc(capacity*sizeof(item_t));

    if(elements_ptr == NULL) {
        free(stack_ptr);
        handle_error("Error while allocating memory.");
    }


    stack_ptr->size = 0;
    stack_ptr->capacity = capacity;
    stack_ptr->elements = elements_ptr;

    return stack_ptr;
}

void delete_stack(stack *mystack)
{
    free(mystack->elements);
    free(mystack);
}

/* Returns 0 on success, 1 on failure */
int push_stack(stack *mystack, item_t data)
{
    /* stack is full! */
    if(mystack->size == mystack->capacity) 
        return 1;

    mystack->elements[mystack->size++] = data;
    return 0;
}

/* Returns a pointer to the popped data or NULL if the stack is empty */
item_t* pop_stack(stack *mystack)
{
    if(mystack->size == 0) {
        return NULL;
    } else {
        return &mystack->elements[--mystack->size];
    }
}

/* Returns a pointer to the adjusted stack or NULL on failure */
stack* resize_stack(stack *mystack, size_t capacity)
{

    item_t *tmp = realloc(mystack->elements, sizeof(item_t) * capacity);
    if(tmp == NULL) 
        return NULL;
    mystack->elements = tmp;
    mystack->capacity = capacity;

    if(mystack->size > capacity)
        mystack->size = capacity;

    return mystack;

}

main.c

#include "stacklib.h"

int main(void){
    stack *my_stack = create_stack(20);
    item_t i;
    for(i = 0; i < 20; i++) {
        push_stack(my_stack, i); 
    }

    resize_stack(my_stack, 10);

    item_t res, *item ;
    while((item = (item_t*)pop_stack(my_stack)) != NULL) {
        res = *item;
        printf("data: %d\n", res);
    }
    delete_stack(my_stack);
    printf("last item popped was  %d\n", res);
    return 0;
}

Answer 1

It looks very nice, code is clean and readable, not much else to change, I think...

---

This is more of a personal preference, not an universal recommendation: One of the things I like about C is having fine control over memory allocations, thus making it a very good choice for systems with limited memory or very rudimentary memory allocators. Because of that, I like to avoid allocating memory whenever possible, so I would not allocate each stack instance on the heap with malloc. Rather, let the user decide where to place it, be on the program stack, as a global or heap-allocated. To do that change, you'd just have to remove that allocation from create_stack and one free from delete_stack, taking a pointer to an external object in create:

bool create_stack(stack* mystack, size_t capacity);

That might seem like a little thing, but forcing the user to dynamically allocate a stack object when it doesn't have to is suboptimal and can be a problem on systems that don't rely on virtual memory and have fragmentation issues.

---

I strongly recommend that you use booleans instead of error codes when a function has only two possible outcomes, success or failure. There's no universal agreement on the meaning of error codes, e.g. some return 0 on success, some return 1, some return -1 on error, and so on. This can be a major source of confusion. Use the bool type from <stdbool.h> whenever it makes sense.

---

Avoid repeating types in your malloc calls. E.g.:

item_t *elements_ptr = malloc(capacity*sizeof(item_t));

Can be refactored to:

item_t *elements_ptr = malloc(capacity * sizeof(*elements_ptr));

Notice that you can use the variable name in the sizeof expression, in this case dereferenced, since we want the size of the type itself, not the size of a pointer. Makes your life easier if you change the name of the type you are allocating; one less place to be updated.

---

Make sure to print errors to stderr. That's the traditional error output and allows users to filter errors from normal program output. Just replace the printf call in handle_error by fprintf(stderr).

---

Two last tips:

You might want to allow the users of your stack to provide their own error handler. You can achieve that with function pointers.

All functions receive pointers to stack, which could be passed in as null by error or due to some memory allocation failure. You can assert that they are not null to trap errors early and ease the debugging efforts.