Generic stack implementation in C

Question

I haven't separated the stack code into its own header and source yet, this is just a proof of concept for now. Haven't chosen better function names yet because I ultimately want to integrate this code into a larger project where I need a way to store data into some kind of dynamic list. Settled for an stack API for now, because of the simplicity.

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

struct CodeGroup
{
    char name[256];
    struct GenericNode *codes;
};

struct Code
{
    char code[8+1]; //each code is supposed to have 8 as max size. +1 for the null terminator
};

struct GenericNode
{
    void *data;
    struct GenericNode *next;
};

void GenericPush(struct GenericNode **list, void *data)
{
    struct GenericNode *new = malloc(sizeof(struct GenericNode));
    new->data = data;
    new->next = NULL;

    if(*list)
        new->next = *list;

    *list = new;

    return;
}

struct GenericNode * GenericPop(struct GenericNode **list)
{
    struct GenericNode *item = NULL;

    if(*list)
    {
        item = *list;
        *list = item->next;
    }

    return item;
}

void GenericFree(struct GenericNode *item)
{
    if(item && item->data)
        free(item->data);
    free(item);

    return;
}

struct CodeGroup *newGroup(char *name)
{
    struct CodeGroup *new = malloc(sizeof(struct CodeGroup));

    strcpy(new->name, name);
    new->codes = NULL;

    return new;
}

struct Code *newCode(char *code)
{
    struct Code *new = malloc(sizeof(struct Code));

    strcpy(new->code, code);

    return new;
}

int main (int argc, char *argv[]) {
    struct GenericNode *groupList = NULL;
    struct GenericNode *item = NULL;
    struct GenericNode *item2 = NULL;
    struct CodeGroup *codeGroup = NULL;
    struct Code *code = NULL;

    /*
         Data is structured as follows:

         group list
            |
            \--- group
            |       \--- name
            |       \--- codes list
            |               \-- code
            |               \-- code
            |               ...
            |
            \--- group
            |       \--- name
            |       \--- codes list
            |               \-- code
            |               \-- code
            |               ...
            |
            ...
    */

    codeGroup = newGroup("group one");
    GenericPush(&codeGroup->codes, (void*)newCode("10000001"));
    GenericPush(&codeGroup->codes, (void*)newCode("10000002"));
    GenericPush(&codeGroup->codes, (void*)newCode("10000003"));
    GenericPush(&codeGroup->codes, (void*)newCode("10000004"));
    GenericPush(&codeGroup->codes, (void*)newCode("10000005"));

    GenericPush(&groupList, (void*)codeGroup);

    codeGroup = newGroup("group two");
    GenericPush(&codeGroup->codes, (void*)newCode("20000001"));
    GenericPush(&codeGroup->codes, (void*)newCode("20000002"));
    GenericPush(&codeGroup->codes, (void*)newCode("20000003"));
    GenericPush(&codeGroup->codes, (void*)newCode("20000004"));
    GenericPush(&codeGroup->codes, (void*)newCode("20000005"));

    GenericPush(&groupList, (void*)codeGroup);

    // prints all groups and its codes, also freeing them in the process:
    while(item = GenericPop(&groupList))
    {
        codeGroup = (struct CodeGroup*)item->data;
        printf("Group name: %s\n", codeGroup->name);
        printf("\'%s\' codes:\n", codeGroup->name);
        while(item2 = GenericPop(&codeGroup->codes))
        {
            code = (struct Code*)item2->data;
            printf("%s\n", code->code);
            GenericFree(item2);
        }
        GenericFree(item);
    }

    return 0;
}

Output:

Group name: group two
'group two' codes:
20000005
20000004
20000003
20000002
20000001
Group name: group one
'group one' codes:
10000005
10000004
10000003
10000002
10000001

Looking for recommendations on how to improve, simplify or standardize this code, or tips on how to make it better overall.

chux · Accepted Answer · 2021-01-22 04:01:04Z

Good formatting

Include.h?

A companion Generic.h with the public functions definitions is a common and good exercise.

Consider a function to test empty-ness

bool GenericEmpty(const struct GenericNode *list);

Consider a function to peek at the top data

void *GenericPeek(const struct GenericNode *list);

Consider a pop that also frees

void *GenericPopData(struct GenericNode **list) {
  void *data = NULL;
  if (list && *list) {
    struct GenericNode *node = *list;
    *list = node->next;
    data = node->data;
    free(node);
  }
  return data;
}

Drop unneeded cast

// GenericPush(&codeGroup->codes, (void*)newCode("10000001"));
GenericPush(&codeGroup->codes, newCode("10000001"));

// code = (struct Code*)item2->data;
code = item2->data;

Allocate per the referenced data

The 2nd form is easier to code right, review and maintain.

// new = malloc(sizeof(struct GenericNode));
new = malloc(sizeof *new);

Robust code checks allocations

p = malloc(...);
if (p == NULL) Handle_Error();

Avoid overruns

// Best way to detect potential overrun is very case dependent.
if (strlen(name) >= sizeof new->name) Handle_error();
else strcpy(new->name, name);

Unneeded test

free(NULL) is OK.

// if(item && item->data)
if(item)
    free(item->data);

Keyword

_Generic is a C keyword. Recommend a different common prefix. Maybe CommonNode?

For me, I tend avoid C++ keywords in C like new, others.

Thank you, great tips! The pop that also frees is a great idea, I don't need the GenericFree() function if pop works that way. Regarding the function that checks for emptiness, can't I just check the list pointer? It will contain NULL if the list is empty. — liewl
– liewl, Commented Jan 22, 2021 at 12:51
@liewl "Regarding the function that checks for emptiness, can't I just check the list pointer? It will contain NULL if the list is empty" --> Yes that is reasonable and idiomatic - if the calling code has awareness of that when struct GenericNode * == NULL (and only then) that that means an empty list. To promote data hiding/abstraction, a define or inline GenericEmpty() could hide that NULL test. Perhaps at a later time, code returns a special pointer to indicate the list is invalid due to out-of-memory. Then the test is list == NULL || list == special. Your call. — chux
– chux, Commented Jan 22, 2021 at 14:09

Stack Exchange Network

Generic stack implementation in C

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Generic stack implementation in C

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions