0
\$\begingroup\$

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.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$
    – liewl
    Jan 22, 2021 at 12:51
  • \$\begingroup\$ @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. \$\endgroup\$ Jan 22, 2021 at 14:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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