After I had my code for stack implementation by array reviewed I wrote stack implementation by using pointers. Here's my code. Any suggestions for improvement are welcome. I'll be adding updated code so please review the latest one. Obviously you are free to ignore this also. To skip reading this long question you can click here.Here's my implementation of stack by using pointers. If someone prefers github here is github#include <assert.h>#include <stdio.h>#include <stdlib.h>enum action {START, PUSH, POP, TOP, QUIT, END};typedef struct node{    int data;    struct node *lower;}stack_node;void clear_screen(void){    system(cls);}static enum action get_user_action(void){    int choice = START;    do    {        clear_screen();        printf(%d Push data\n               %d Pop Data\n               %d See the top of the stack\n               %d Exit\n\n               Enter your choice -> , PUSH, POP, TOP, QUIT);        scanf(%d, &choice);    } while (!(START < choice && choice < END));    return (enum action) choice;}void push(stack_node **top_stack, int *status, int data){    *status = START;    stack_node *node = malloc(sizeof(node));    if (node == NULL)    {        *status = PUSH;        return;    }    node -> data = data;    if (*top_stack == NULL){        node -> lower = NULL;    }    else{        node -> lower = *top_stack;    }    *top_stack = node;}int pop(stack_node **top_stack, int *status){    *status = START;    if (*top_stack == NULL){        *status = POP;        return -1;    }    stack_node *node = *top_stack;    int data = node -> data;    *top_stack = node -> lower;    free(node);    return data;}int peek(stack_node **top_stack, int *status){    *status = START;    if (*top_stack == NULL){        *status = POP;        return -1;    }    return (*top_stack) -> data;}int main(void){    enum action choice;    int status;    stack_node *top = NULL;    while ((choice = get_user_action()) != QUIT)    {        clear_screen();        int data;        switch (choice)        {        case PUSH:            printf(Enter data to be pushed -> );            scanf(%d, &data);            push(&top, &status, data);            if (status == PUSH){                printf(Not enough memory\n);            }            else{                printf(%d pushed onto the stack, data);            }            break;        case POP:            data = pop(&top, &status);            if (status == POP){                printf(Stack underflow\n);            }            else{                printf(The data is %d\n, data);            }            break;        case TOP:            data = peek(&top, &status);            switch (status)            {            case POP:                printf(Nothing in the stack\n);                break;            default:                printf(The data at top is %d\n, data);            }            break;        default:            assert(!You should not have reached this.);        }        getchar();        getchar();    }}Update 1I think I have made the stack able to use any data type. This is what I have made. I also added a function for deleting the stack stack_delete.#include <assert.h>#include <stdio.h>#include <stdlib.h>#include <string.h>enum action {START, PUSH, POP, TOP, LENGTH, QUIT, END};enum status {SUCCESS, FAILURE};typedef struct node {    void *data;    struct node *lower;} stack_node;typedef struct stack {    size_t elem_size;    size_t stack_size;    stack_node *top;} stack_struct;void clear_screen(void){    system(cls);}static enum action get_user_action(void){    int choice = START;    do {        clear_screen();        printf(%d Push data\n               %d Pop Data\n               %d See the top of the stack\n               %d See the length of the stack\n               %d Exit\n\n               Enter your choice -> , PUSH, POP, TOP, LENGTH, QUIT);        scanf(%d, &choice);    } while (!(START < choice && choice < END));    return (enum action) choice;}enum status stack_create(stack_struct **stack, size_t elem_size){    (**stack).elem_size = elem_size;    (**stack).stack_size = 0;    (**stack).top = NULL;    return SUCCESS;}enum status push(stack_struct **stack, void *data){    stack_node *node = malloc(sizeof(node));    if (node == NULL) {        return FAILURE;    }    node->data = malloc(sizeof((**stack).elem_size));    if (node->data == NULL) {        return FAILURE;    }    memcpy(node->data, data, (**stack).elem_size);    node->lower = (**stack).top;    (**stack).top = node;    (**stack).stack_size += 1;    return SUCCESS;}enum status pop(stack_struct **stack, void *data){    if ((**stack).top == NULL) {        return FAILURE;    }    stack_node *node = (**stack).top;    memcpy(data, node->data, (**stack).elem_size);    (**stack).top = node->lower;    free(node->data);    free(node);    (**stack).stack_size -= 1;    return SUCCESS;}enum status peek(stack_struct **stack, void *data){    if ((**stack).top == NULL) {        return FAILURE;    }    memcpy(data, (**stack).top->data, (**stack).elem_size);    return SUCCESS;}void stack_delete(stack_struct **stack){    while ((**stack).top != NULL)    {        stack_node *node = (**stack).top;        (**stack).top = (**stack).top->lower;        free(node->data);        free(node);    }    free(*stack);}int main(void){    enum action choice;    stack_struct *stack = malloc(sizeof(stack_struct));    if (stack == NULL)    {        printf(Not enough memory\n);        return 1;    }    stack_create(&stack, sizeof(int));    while ((choice = get_user_action()) != QUIT) {        clear_screen();        int data;        switch (choice) {            case PUSH:                printf(Enter data to be pushed -> );                scanf(%d, &data);                if (push(&stack, &data) == SUCCESS) {                    printf(%d pushed onto the stack\n, *(int *)stack->top->data);                } else {                    printf(Not enough memory\n);                }                break;            case POP:                if (pop(&stack, &data) == SUCCESS){                    printf(The data is %d\n, data);                } else {                    printf(Stack underflow\n);                }                break;            case TOP:                if (peek(&stack, &data) == SUCCESS){                    printf(The data at top is %d\n, data);                } else {                    printf(Nothing in the stack\n);                }                break;            case LENGTH:                printf(Length is %d\n, stack->stack_size);                break;            default:                assert(!You should not have reached this.\n);        }        getchar();        getchar();    }    stack_delete(&stack);}Update 3I added it after Corbin's review of my Update 2. So far I have just left the use of linked list for implementing the stack. I haven't made 2 files yet but I have placed stack implementation at the top of the file for showing the logical separation of implementation code and client code. As this isn't a very big implementation I think the logical separation is what is needed here.Here is my updated code.#include <assert.h>#include <stdio.h>#include <stdlib.h>#include <string.h>enum status {STACK_SUCCESS, STACK_FAILURE};typedef struct node {    void *data;    struct node *lower;} stack_node;typedef struct stack {    size_t elem_size;    size_t stack_size;    stack_node *top;} STACK;enum status stack_init(STACK *stack, size_t elem_size){    stack->elem_size = elem_size;    stack->stack_size = 0;    stack->top = NULL;    return STACK_SUCCESS;}stack_node * stack_node_create(STACK *stack){    stack_node *node = malloc(sizeof(stack_node));    if (node == NULL) {        return NULL;    }    node->data = malloc(stack->elem_size);    if (node->data == NULL) {        free(node);        return NULL;    }    return node;}enum status stack_push(STACK *stack, void *data){    stack_node *node = stack_node_create(stack);    if (node == NULL) {        return STACK_FAILURE;    }    memcpy(node->data, data, stack->elem_size);    node->lower = stack->top;    stack->top = node;    stack->stack_size += 1;    return STACK_SUCCESS;}enum status stack_pop(STACK *stack, void *data){    if (stack->top == NULL) {        return STACK_FAILURE;    }    stack_node *node = stack->top;    memcpy(data, node->data, stack->elem_size);    stack->top = node->lower;    free(node->data);    free(node);    stack->stack_size -= 1;    return STACK_SUCCESS;}enum status stack_peek(STACK *stack, void *data){    if (stack->top == NULL) {        return STACK_FAILURE;    }    memcpy(data, stack->top->data, stack->elem_size);    return STACK_SUCCESS;}void stack_cleanup(STACK *stack){    while (stack->top != NULL) {        stack_node *node = stack->top;        stack->top = stack->top->lower;        free(node->data);        free(node);    }}//Stack Implementation before this.//Things that are used to use the stack are placed after this.enum action {START, PUSH, POP, TOP, LENGTH, QUIT, END};void clear_screen(void){    system(cls);}static enum action get_user_action(void){    int choice = START;    do {        clear_screen();        printf(%d Push data\n               %d Pop Data\n               %d See the top of the stack\n               %d See the length of the stack\n               %d Exit\n\n               Enter your choice -> , PUSH, POP, TOP, LENGTH, QUIT);        scanf(%d, &choice);    } while (!(START < choice && choice < END));    return (enum action) choice;}int main(void){    enum action choice;    STACK stack;    stack_init(&stack, sizeof(int));    while ((choice = get_user_action()) != QUIT) {        clear_screen();        int data;        switch (choice) {            case PUSH:                printf(Enter data to be pushed -> );                scanf(%d, &data);                if (stack_push(&stack, &data) == STACK_SUCCESS) {                    printf(%d pushed onto the stack\n, *(int *)stack.top->data);                } else {                    printf(Not enough memory\n);                }                break;            case POP:                if (stack_pop(&stack, &data) == STACK_SUCCESS){                    printf(The data is %d\n, data);                } else {                    printf(Stack underflow\n);                }                break;            case TOP:                if (stack_peek(&stack, &data) == STACK_SUCCESS){                    printf(The data at top is %d\n, data);                } else {                    printf(Nothing in the stack\n);                }                break;            case LENGTH:                printf(Length is %d\n, stack.stack_size);                break;            default:                assert(!You should not have reached this.\n);        }        getchar();        getchar();    }    stack_cleanup(&stack);}
Review implementation of stack by using pointers in C
c;stack;pointers
Lstor has most of these, but I'd like to go into more detail on a few things and also suggest a few new things.As Lstor said, separate your declarations and your implementation. Declarations go in a header file (.h), and definitions go in a source file (.c). Consuming code then includes the header and doens't have to even know the source file exists. In fact, for non-trivial libraries, the source is rarely used directly. Instead, the header is used and the consuming code is linked against a library. You don't need to go that far since your library is small and has a trivial compilation process, but the separation of declaration and implementation is definitely worth keeping in mind.Create a naming scheme and go with it. I suggest library_component_function, like ab_stack_push (ab being for Aseem Bansal in this example). In a way, it's essentially namespacing via name. Provided you pick a reasonable prefix, no other library is going to have used it yet, meaning you'll have the entire space of name_ to yourself. (Avoid well know libraries' prefixes like curl_, apr_, ev_, etc).Even if you don't do a library prefix, you should prefixing the functions with stack_. Not only does any kind of reusable code need a prefix or risk an inevitable name clash, it's also just simpler to know that all stack related functions start with stack. It makes it easier to find and remember the functions.As Lstor hinted at, I would create a different, more versatile data structure and implement your stack in terms of that. While a pure stack is going to be a bit more efficient memory-wise, using a different data structure to implement it means less code to manage and ease of design. I would implement a stack as either a vector or a doubly linked list. In either situation, you're basically taking a more complete data type and removing functionality. That means that all of your stack code can just be thin wrappers.I'm always tempted to impelement data structures as an opaque pointer. By only exposing the struct as a pointer, you can ensure that client code never touches the internals of it. It's quite handy for completely hiding an implementation, and it has a few other minor benefits, but it also comes at a cost of the struct always having to be dynamically allocated. (Well, technically it doesn't have to be, but it's quite a pain otherwise).The basic idea is to forward declare a struct in the header and only actually define it in the implementation file. Forward declared types can have a pointer to them (since the size of a pointer is always known), but the plain type cannot be used until the full declaration has been provided. This means that only your implementation can use the contents of the struct since consuming code won't know what's in it. Consuming code can only see a pointer to the struct.This is a pattern you've probably actually seen before if you've used anything that works like a handle. For example, FILE is an opaque pointer in GNU's libc. Consuming code only ever works with FILE in the sense of FILE*. You would never have a plain FILE, and, in fact, it's not possible with GNU's libc.Rather than a human driven test, consider a semi-rigourous, programmatic test (think unit testing). Yeah, it's cool to have a little program that lets you use your hard work and see it in action, but in terms of actual bug catching, you want something you can instantly reproduce, not go wait... how many elements had I inserted? And what were they? Rather, it'd be nice if your test found a bug in your stack if you could simply run it again to recreate said bug. If you haven't looked into unit testing, I'm essentially recommending a simplified version of that.From a usability perspective, genericness is crucial. A stack that can only store ints is pretty useless. I'd like to expand on Lstor's advice to use void*. A good introduction to it is here http://www.youtube.com/watch?v=iyLNYXcEtWE&feature=PlayList&p=9D558D49CA734A02&index=5. The basics of it are pretty simple though once you wrap your head around it. By using a void*, you essentially treat blocks of memory as blocks of memory and nothing more. A good example of this is void* memcpy(void* dest, const void* src, std::size_t count); in the standard. memcpy doesn't care what the memory is. It doesn't care if it's an int*, a char** or whatever. It just knows that you've handed it a chunk of memory and told it to copy count bytes of it to another block of memory. Your stack implementation would be similar: you just hand it a chunk of memory and tell it to push it. By keeping size of each element in the stack_t struct, your method won't require the size. In other words, void stack_push(stack_t* stack, int val); becomes int stack_push(stack_t* stack, void* val); and stack_t* stack_create(); becomes stack_t* stack_create(size_t elem_size);. The reason the size is stored internally rather than provided each time is two fold. First is for usability purposes, but second is that you want to see your instance as a stack of something. You want stack_t a* = stack_create(sizeof(int)); to be a stack of ints and your stack_t* b = stack_create(sizeof(char*)); to be a stack of char* (example names, not a suggested naming scheme :p). Ideally they'd be tied to that one type and that one type only. Ideally they'd be part of the actual type of the object (looks longingly at C++). That can't happen in C though, so instead, we leave the sizing business to the internals and we trust ourselves to only ever pass it the type it is containing.As for the acutal usuage, the main confusion with void* is what the hell do I do with a void*?. void* is just used to accept or return a pointer of unknown type. To actually work with it, you will cast it to a char* and use it. Casting it to a char* allows you to do single-byte arithmetic on it. Since you already know the size of each element, you can use that to multiply out single byte arithmetic. This is essentially what the compiler does for you when you use typed pointers. With int* ints, ints + 3 is the same thing as ((char*) ints) + sizeof(int) * 3. Since pointer is still typed, it actually is still inserting the size for you. It just happens that sizeof(char) is always 1.Imagine you have a void* ptr that you know holds 20 elements that are each 4 bytes. This means that element n is at void* elem_n = ((char*) ptr) + (4 * n);. This is really the entire concept. You know how to find addresses, and you know what the sizes are. That's all you need :). In fact, if you're working with linked structures, you don't even need to know how to find offset addresses. You just need to know to use the stored size.An example:typedef struct {    stack_node_t* prev;    void* data;} stack_node_t;typedef struct {    size_t elem_size;    size_t size;    stack_node_t* top;} stack_t; //_t suffixed names are all reserved by POSIX.            //In reality, that won't matter for a type            //like `stack_t`, but worth keeping in mind.stack_t* stack_create(size_t elem_size){    stack_t* stack = malloc(sizeof(*stack));    if (stack == NULL) { return NULL; }    stack->elem_size = elem_size;    stack->size = 0;    stack->top = NULL;    return stack;}int stack_push(stack_t* stack, void* elem){    stack_node_t* node = malloc(sizeof(*node));    if (node == NULL) {        return STACK_ERR_ALLOC;    }    node->data = malloc(stack->elem_size);    if (node->data == NULL) {        return STACK_ERR_ALLOC;    }    memcpy(node->data, elem, stack->elem_size);    node->prev = stack->top;    stack->top = node;    return STACK_SUCCESS;}An example that uses an array might be a bit more helpful, so if you'd like one of those, let me know (or, watch the earlier linked video -- it implements an array based stack).