0
\$\begingroup\$

I was bored so I crapped out a simple game that responds to commands and manipulates an inventory.

I am interested in other possible ways to handle the inventory, and I think I went overboard with the linked lists

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

#include <time.h>
#include <editline/readline.h>
#include <editline/history.h>

typedef struct command_args {
    struct command_args* next;
    char* arg;
} command_args_t;


void command_add_item(command_args_t, uint8_t);
void command_show_inventory(command_args_t, uint8_t);
void command_bye(command_args_t, uint8_t);

typedef void (*command_func_t)(command_args_t, uint8_t);


#define COMMAND_NUMBER 3
char* command_table[] = {
    "add",
    "inventory",
    "bye"
};

command_func_t command_func_table[] = {
    &command_add_item,
    &command_show_inventory,
    &command_bye
};

#define ADD_ARGS 4

#define WEAPON 0x0001
#define ARMOR  0x0002
#define MAGIC  0x0004
#define MISC   0x0008

typedef struct items {
    struct items*   next;
    char*           name;
    uint16_t        price;
    uint8_t         weight;
    uint8_t         type;
} items;

typedef struct player {
    uint16_t coins;
    uint8_t  health;
    items    i;
} player;

player hero;

items* inventory_current = &hero.i;
void add_item(char* name, uint16_t price, uint8_t weight, uint8_t type) {
    inventory_current->next = (items*)malloc(sizeof(items));
    inventory_current->next->name   = strdup(name);
    inventory_current->next->price  = price;
    inventory_current->next->weight = weight;
    inventory_current->next->type   = type;
    inventory_current->next->next   = NULL;

    inventory_current = inventory_current->next;

    return;
}

void command_add_item(command_args_t args, uint8_t argc) {
    if(argc < ADD_ARGS) {
        printf("[!] you're missing something\n");
        return;
    }

    printf("\tName: %s\n", args.arg);
    printf("\tPrice: %s",  args.next->arg);
    printf("\tWeight: %s", args.next->next->arg);
    printf("\tType: %s\n", args.next->next->next->arg);

    add_item(args.arg, atoi(args.next->arg), atoi(args.next->next->arg), atoi(args.next->next->next->arg));

    return;
}

void show_inventory() {
    items* current = &hero.i;
    for(; current; current = current->next) {
        printf("\tINVENTORY\n");
        printf("\t\tName: %s\n", current->name);
        printf("\t\tPrice: %d", current->price);
        printf("\t\t Weight: %d", current->weight);
        printf("\t\tType: %d\n", current->type);
    }

    return;
}

void command_show_inventory(command_args_t _args, uint8_t _argc) {
    show_inventory();
}

void command_bye(command_args_t _args, uint8_t _argc) {
    exit(0);
}


void process_input(char* input) {
        char* term;
        command_func_t command = NULL;
        command_args_t args;
        uint8_t argc;

        term = strtok(input, " ");

        for(size_t i = 0; i < COMMAND_NUMBER; i++) {
            if(strcmp(term ,command_table[i]) == 0) {
                command = command_func_table[i];
                break;
            }
        }

        if(!command) {
            printf("How should I know how to do that?\n");
            return;
        }

        args.next = malloc(sizeof(command_args_t));
        command_args_t* current = &args;
        for(argc = 0; term = strtok(NULL, " "); argc++) {
            current->next = malloc(sizeof(command_args_t));
            current->arg = term;
            current = current->next;
        }
        current->next = NULL;

        command(args, argc);
}

int main() {
    srand(time(NULL));

    hero.health   = 100;
    hero.coins    = 10;
    hero.i.name   = "Inventory";
    hero.i.weight = 0;
    hero.i.price  = 0;
    hero.i.type   = MISC;
    hero.i.next   = NULL;

    //add_item("Inventory", 0, 0, MISC);

    char* input;
    while(input = readline("> ")) {
        if(input == "") continue;
        add_history(input);
        process_input(input);
        free(input);
    }
}
\$\endgroup\$
3
\$\begingroup\$
  • process_input leaks memory like there is no tomorrow. Memory allocated with current->next = malloc(sizeof(command_args_t)); is never released.

    On top of that, the sequence

            args.next = malloc(sizeof(command_args_t));
            command_args_t* current = &args;
            for(argc = 0; term = strtok(NULL, " "); argc++) {
                current->next = malloc(sizeof(command_args_t));
    

    allocates args.next and immediately overwrites this pointer on the very first iteration. Initializing with just args.next = NULL; is more prudent.

  • Having parallel command_table and command_func_table is prone to errors. As the number of commands grows it is easy to lose the sync. I recommend to have a single table of

    struct command {
        char * command_name;
        command_func_t command_func;
    };
    

    For the same reason of avoiding double maintenance, do not hardcode COMMAND_NUMBER. Either guard the table by the NULL command name, or compute its size at the compile time with the count_of. If your compiler does not support count_of, define it yourself as

    #define count_of(arr) (sizeof(arr) / sizeof(arr[0]))
    

    As a side note, consider keeping command names alphabetically sorted, to binary search them. As the list of commands grows, you would feel the difference.

  • I don't see how WEAPON and friends are used.

  • I think we all agree that atoi(args.next->next->next->arg) smells.

    Besides, it can easily segfault (because process_input is unaware of how many arguments the command needs), or produce incorrect result (because process_input is unaware of argument types).

    I recommend to let the command parse its arguments (rather than do it in process_input). process_input would only parse the command name, and pass the rest of input to the command to process. Or use the getopt library.

  • It feels very uncomfortable having hero and current_inventory as independent globals. Consider adding an items * last_item; field to struct player.

\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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