Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 1

I 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 3

I 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);
}
share|improve this question
    
Is this meant to be reviewed as library style code or as a one-off intellectual oversize? In other words, would you like for us to critique the external-developer usability of this, or just critique it for being a simple implementation and demo? –  Corbin Jul 31 '13 at 10:40
    
@Corbin Review according to reusability. I think that would mean external-developer usability. –  Aseem Bansal Jul 31 '13 at 10:49
    
You might find this code of mine interesting. It implements a stack with a very generic interface. The client code which uses the stack, just passes a void* and saves the result on every call. This void* points to the stack state, but the client code cannot access it because the definition is not in the header file, just the functions that can be used. $0.02. :) –  luser droog Aug 2 '13 at 2:25
2  
Isn't it better to start a new review for the updated code? –  Lstor Aug 4 '13 at 14:41
    
@Lstor Just wanting to know whether it is good or not. Starting a new one each time I update it would lead to something like 7 copies of the same thing. Also if I have missed something then people will be repeating themselves. That might be annoying for the reviewers as well as any readers. Also for iterative reviews the only thing that can successfully work in the current structure without making a mess of this website would be adding the code. Meta discussion. –  Aseem Bansal Aug 4 '13 at 15:31

3 Answers 3

up vote 3 down vote accepted

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).

share|improve this answer
    
Hats off! I particularly liked forcing use of pointers, I haven't thought of or seen that approach before. I also didn't know _t is reserved by POSIX. –  Lstor Jul 31 '13 at 20:12
    
@Lstor it has a few downsides, especially in certain scenarios (embedded systems come to mind where mallocing a pointer might be more than a minor inconvenience), but it can be nice to completely hide implementation. And yeah, _t is reserved by POSIX. I can't imagine POSIX would ever add a stack, so it's likely fine in this context, but by the letter of the law, it's technically wrong on any POSIX system. –  Corbin Jul 31 '13 at 21:47
    
I think I have made the stack able to use any data type. Please have a look. –  Aseem Bansal Aug 4 '13 at 11:49

First of all, you should put the implementation in a separate implementation file and expose access to it through a header.

Second, your stack_node is an implementation detail and should not be exposed to client code. Instead, create a struct stack that contains bookkeeping data and a pointer to the top node:

typedef struct {
    size_t length;
    stack_node *top;
} stack_t;

Make functions to create and delete (free) such stack_ts. All your functions should take a pointer to the stack_t they operate on, and should not take any further bookkeeping data.

Usage example:

stack_t *values = stack_create();

stack_push(values, 42);
int ultimate_answer = stack_peek(values);
stack_push(values, 9*6);

int no_jokes_in_base_thirteen = stack_pop(values);

stack_delete(values);

I would also make a helper function to create nodes. It should not be used by client code, and therefore it should have internal linkage (i.e. be declared static) in the stack implementation file.

Other comments:

  • Don't have spaces when using ->, just like you wouldn't use spaces when using .:

    node->lower = *top_stack; // node is pointer
    node.lower = *top_stack;  // node is not pointer
    
  • Be consistent with your style. You are mixing K&R-style and Allman/BSD-style, i.e.

    if (kernighan_and_ritchie) {
    }
    

    vs.

    if (bsd_style)
    {
    }
    

    Pick one and stick to it. I personally prefer clean K&R style, which also uses "cuddled else":

    if (cuddly) {
    } else {
    }
    

    But having a newline before the else is also fine.

  • I'd rename top_stack to top_of_stack, stack_top or simply top, otherwise it can give the impression of being a stack.

Edit:

A few things I forgot:

  • To be useful to other developers, the stack would need to take other types than just ints.

  • Your stack is really a singly linked list with some limitations. It would be a good exercise to implement a complete implementation of a singly (and later, doubly) linked list.

  • Your implementation does not have a way to get the size of the stack. size should be O(1). That means that the stack should keep track of its own size, like I indicated in the stack_t example above.

share|improve this answer
    
I think can take care of everything except one. How do I make the stack to take different data types? Data types are decided at compile time, if I am correct. I know that stdlib.h has qsort and bsearch which can take any type but I don't know how that could be implemented. Can you provide some hint about how to implement that? –  Aseem Bansal Jul 31 '13 at 11:24
    
Use void*. Dennis Ritchie said that "C is a strongly typed, weakly checked language", and void* is very weakly checked. But that increases the complexity, so I recommend you wait until you are comfortable with void*. –  Lstor Jul 31 '13 at 11:31
    
I think I have made the stack able to use any data type. Please have a look. –  Aseem Bansal Aug 4 '13 at 11:48

A few things with regards to your new code:


SUCCESS and ERROR are very risky enum identifiers. Since enums share a global scope, this means that no code that your code is used with can ever use either of those identifiers in an enum or your code won't compile. That's a big risk to take. Just be to on the safe side, you could prefix them STACK_SUCCESS/STACK_ERROR.


Some functions have stack_ prefix and some don't. All of them should have the prefix.


There's still no separation of declaration and implementation (header/source). Normally this wouldn't be an issue since it's such a small script, but you said that you'd like it reviewed according to reusability. Reusability hinges on a well defined separation.


All of your stack** should just be stack*.

None of your double pointers are actually being used a double pointer, so there's no purpose to have the confusion/indirection overhead.

Also, once it's a stack*, you main will just need a stack so you won't have to malloc it.


stack_create should be named stack_init. create implies that memory is allocated. init implies that a structure is initialized. new and create tend to imply that both memory is allocated and initialized. init implies that initialization is done on existing memory.


There's a mismatch in memory semantics. The consuming code allocates the stack, but the stack_delete method destroy it. If the client mallocs it, the client should free it. Either stack_create needs to malloc the stack, or stack_delete needs to not free it (and if it doesn't free, it should be renamed to something like stack_dispose or stack_cleanup).


I don't like stack_struct for two reasons: it's obvious by intuition/IDE inspection/reading the header that it's a struct. You never see x_int or x_char for a reason: it's useless.

The more important reason though is that the end user doesn't need to know that it's a struct. In fact, if the end user could just see it as a black-box style little object, that would be for the best. It being a struct is an implementation detail, and you typically want to hide implementation details from consumers of your code.


stack_node *node = malloc(sizeof(node)); is probably a typo. This is the same as stack_node *node = malloc(sizeof(stack_node*)); since it's not struct node. It should likely be stack_node *node = malloc(sizeof(*node));


You have a potential memory leak in push. What happens if the node is successfully mallocd, but the node->data malloc fails? Once push returns, you no longer have a pointer to node which means that there's now memory that is allocated but not pointed to. There's a lot of ways you can handle code flows like this. In this particular situation, I would do something like this:

enum status push(stack_struct *stack, void *data)
{
    stack_node *node = malloc(sizeof(*node));

    if (!node) {
        return FAILURE;
    }

    node->data = malloc(sizeof(stack->elem_size));
    if (node->data) {
        memcpy(node->data, data, stack->elem_size);
        node->lower = stack->top;
        stack->top = node;
        stack->stack_size += 1;
        return SUCCESS;
    }

    free(node);

    return FAILURE;
}

I'd be tempted to pull the node management out into its own functions. At the moment it doesn't really matter since the code is so small. It might simplify certain things though (like the example above). The node management would of course be implemented as internal linkage only inside of the stack source so external code can't see it (static functions).

As an example of simplification, consider a new implementation of push:

static stack_node* stack_node_create(void* data, size_t elem_size)
{
    stack_node* node = malloc(sizeof(*node));
    if (!node) { return NULL; }
    node->data = malloc(elem_size);
    if (!node->data) {
        free(node);
        return NULL;
    }
    memcpy(node->data, data, elem_size);
    return node;
}

static void stack_node_destroy(stack_node* node)
{
    free(node->data);
    free(node);
}

enum status push(stack_struct *stack, void *data)
{
    stack_node* node = stack_node_create(data, stack->elem_size);
    if (!node) {
        return FAILURE;
    }
    node->lower = stack->top;
    stack->top = node;
    stack->stack_size += 1;
    return SUCCESS;
}
share|improve this answer
    
I didn't like stack_struct either but as you told x_t is reserved for POSIX and I didn't want to form a bad habit even though it is valid in this case. I guess I'll use STACK instead of stack_struct(Inspiration from FILE). Other than that my focus currently was to make it work. The next time I update I'll take care of everything said so far(except the use of linked list for stack as I have reserved that for future). –  Aseem Bansal Aug 5 '13 at 5:43
    
I just noticed that your example's stack_node_destroy is redundant. The function stack_node_create actually takes care of everything. There is no memory to be freed if node == NULL. But good point about pulling out node management into a separate function. Also about your stack_create_function I was suggested that returning status would be a better idea compared to returning the data(I think that data here is the pointer). Is this better for functions called by the client or is it useful practice in libraries also? –  Aseem Bansal Aug 5 '13 at 5:56
    
I made Update 2 according to suggestions given. I haven't made 2 files but I have shown a clear logical separation of the implementation and the client code. –  Aseem Bansal Aug 5 '13 at 6:37
    
@AseemBansal I would just stick would stack or even the old stack_struct in favor of STACK. All caps seems odd and is a pain to type. How is stack_node_destroy redundant? It's meant to be used when destroying the list. It has nothing to do with cleanup when allocating the nodes (it just encapsulates allocation of the node and the data as a pseudo-atomic operation from the outside). –  Corbin Aug 5 '13 at 7:59
    
As for the return being a pointer... Since it's an internal function it doesn't matter. If you really wanted, you could pass a stack_node** and return a status flag instead of returning a pointer. At the end of the day though, the function will either succeed in allocating or fail. A pointer can conveniently encode this flag as NULL or non-NULL. If it were external linkage, there might be an argument for a status return (in future, more than 1 error reason might exist for failure). Since it's internal though, I wouldn't bother. Internal things can be easily-ish changed if/when necessary. –  Corbin Aug 5 '13 at 8:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.