4
\$\begingroup\$

I have finished a school project recently which was a recipe manager to be implemented in C. Coming from C# I am really struggling on getting used to the programming style of C. I feel like the code is super clumsy and not really neat. They've taught us that potential errors should always be caught. This is why I came up with:

void* safe_malloc(size_t size) {
    void* ptr = malloc(size);

    if (ptr == NULL) {
        fprintf(stderr, "Memory allocation failed.\n");
        exit(EXIT_FAILURE);
    }

    return ptr;
}

Anytime my program fails to allocate memory, there is no way to recover from it - I'm just out of memory, right? Is there any possibility you could recover from such a scenario?

I've uploaded the project to my repository https://github.com/ViolentOnion/RecipeManager and I would welcome any type of constructive feedback and or tips/tricks.

FileManager.c:

#include "FileManager.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "Utilities.h"

Recipe* parseRecipes(const char* path) {
    if (path == NULL) return NULL;

    FILE* fp = fopen(path, "r");
    char* lineBuff = (char*)safe_malloc(sizeof(char) * LINE_BUFFER_SIZE);
    Recipe* start = NULL;

    if (fp == NULL) {
        fprintf(stderr, "Could not open file at path %s\n", path);
        exit(EXIT_FAILURE);
    }

    while (fgets(lineBuff, LINE_BUFFER_SIZE, fp) != NULL) {
        // skip empty lines
        if (strcmp(lineBuff, "\n") == 0 || strcmp(lineBuff, "\r\n") == 0) {
            continue;
        }

        char* name = strtok(lineBuff, FILE_DELIMITER);
        char* ingredientString = strtok(NULL, FILE_DELIMITER);
        char* instructions = strtok(NULL, FILE_DELIMITER);

        if (name == NULL || ingredientString == NULL || instructions == NULL) {
            fprintf(stderr, "Invalid file structure.\n");
            exit(EXIT_FAILURE);
        }

        if (strtok(NULL, FILE_DELIMITER) != NULL) {
            fprintf(stderr, "Only three tokens allowed.\n");
        }

        Ingredient* ingredients = parseIngredients(ingredientString);
        start = insertRecipe(start, name, instructions, ingredients);
    }

    free(lineBuff);
    fclose(fp);

    return start;
}

Ingredient* parseIngredients(char* ingredients) {
    if (ingredients == NULL) return NULL;

    char* ptr;
    char* token = strtok_r(ingredients, INGREDIENT_DELIMITER, &ptr);

    if (token == NULL) {
        fprintf(stderr, "Invalid ingredient structure.\n");
        exit(EXIT_FAILURE);
    }

    Ingredient* start = NULL;

    do {
        char* ingrPtr;
        char* amountStr = strtok_r(token, INGREDIENT_COMP_DELIMITER, &ingrPtr);
        char* unit = strtok_r(NULL, INGREDIENT_COMP_DELIMITER, &ingrPtr);
        char* name = strtok_r(NULL, INGREDIENT_COMP_DELIMITER, &ingrPtr);

        if (amountStr == NULL || unit == NULL || name == NULL) {
            fprintf(stderr, "Error while parsing ingredients.\nPlease check the file structure.\n");
            exit(EXIT_FAILURE);
        }

        int amount = (int)strtol(amountStr, NULL, 10);

        if (amount == 0 && strcmp(amountStr, " ") != 0) {
            fprintf(stderr, "Error while parsing ingredient amount\nPlease check the file structure.\n");
            exit(EXIT_FAILURE);
        }

        start = insertIngredient(start, amount, name, unit);
    } while ((token = strtok_r(NULL, INGREDIENT_DELIMITER, &ptr)) != NULL);

    return start;
}

int writeToFile(Recipe* start, const char* path) {
    if (start == NULL || path == NULL) return -1;

    int rowsWritten = 0;
    Recipe* recipe = start;
    FILE* fp = fopen(path, "w");

    if (fp == NULL) return -1;

    while (recipe != NULL) {
        if (fprintf(fp, "%s;", recipe->name) <= 0) {
            return -1;
        }

        Ingredient* curr = recipe->ingredients;

        while (curr != NULL) {
            if (curr->amount == 0) {
                if (fprintf(fp, " |") <= 0) return -1;
            }
            else {
                if (fprintf(fp, "%d|", curr->amount) <= 0) return -1;
            }

            if (strcmp(curr->unit, "\0") == 0) {
                if (fprintf(fp, " |") <= 0) return -1;
            }
            else {
                if (fprintf(fp, "%s|", curr->unit) <= 0) return -1;
            }

            if (fprintf(fp, "%s#", curr->name) <= 0) {
                return -1;
            }

            curr = curr->next;
        }

        if (fprintf(fp, ";%s", recipe->instructions) <= 0) return -1;

        recipe = recipe->next;
        rowsWritten++;
    }

    fclose(fp);

    return rowsWritten;
}

Ingredient.c

#include "Ingredient.h"
#include "../Utilities.h"
#include <stdio.h>
#include <string.h>

Ingredient* insertIngredient(Ingredient* start, unsigned int amount, const char* name, const char* unit) {
    Ingredient* new = (Ingredient*)safe_malloc(sizeof(Ingredient));

    new->name = (char*)safe_malloc(sizeof(char) * (strlen(name) + 1));
    strcpy(new->name, name);

    new->unit = (char*)safe_malloc(sizeof(char) * (strlen(unit) + 1));
    strcpy(new->unit, unit);

    new->amount = amount;
    new->next = NULL;

    if (start == NULL) return new;

    Ingredient* temp = start;

    while (temp->next != NULL) {
        temp = temp->next;
    }

    temp->next = new;

    return start;
}


void prettyPrintIngredients(Ingredient* ingredients) {
    if (ingredients == NULL) return;

    printf("Ingredients:\n");

    Ingredient* current = ingredients;

    while(current != NULL) {
        prettyPrintIngredient(current);
        current = current->next;
    }

    printf("\n");
}

void prettyPrintIngredient(Ingredient* ingredient) {
    if (ingredient == NULL) return;

    if (ingredient->amount == 0) {
        printf("%s\n", ingredient->name);
        return;
    }

    printf("%-4d  %-10s  %s\n", ingredient->amount, ingredient->unit, ingredient->name);
}

void freeIngredient(Ingredient* ingredient) {
    if (ingredient == NULL || ingredient->next == NULL) return;

    freeIngredient(ingredient->next);
    free(ingredient->name);
    free(ingredient->unit);
    free(ingredient);
}

Ingredient* readIngredients(unsigned int amount) {
    char buff[20];
    char name[20];
    char unit[20];
    unsigned int ingrCount = 0;
    Ingredient* new = NULL;

    while (ingrCount < amount) {
        fprintf(stdout, "Please enter the name of the ingredient or type (q) to quit:\n");

        if (fgets(buff, sizeof(buff), stdin) == NULL || strcmp(buff, "\n") == 0) {
            fprintf(stdout, "Please enter a non-empty name.\n");
            continue;
        }

        if (strcmp(buff, "q\n") == 0) return NULL;

        buff[strcspn(buff, "\n")] = '\0';
        strcpy(name, buff);

        fprintf(stdout, "Please enter an amount (leave blank for nothing): \n");
        if (fgets(buff, sizeof(buff), stdin) == NULL) {
            fprintf(stderr, "Error while reading input.\n");
            continue;
        }

        int ingrAmount = (int)strtoul(buff, NULL , 10);

        fprintf(stdout, "Please enter a unit (leave blank for nothing): \n");
        if (fgets(buff, sizeof(buff), stdin) == NULL) {
            fprintf(stderr, "Error while reading input.\n");
            continue;
        }

        buff[strcspn(buff, "\n")] = '\0';
        strcpy(unit, buff);

        new = insertIngredient(new, ingrAmount, name, unit);
        ingrCount++;
    }

    return new;
}

Recipe.c

#include "Recipe.h"
#include "../Utilities.h"
#include <string.h>

Recipe* insertRecipe(Recipe* start, const char* name, const char* instructions, Ingredient* ingredientList) {
    Recipe* new = (Recipe*)safe_malloc(sizeof(Recipe));

    new->index = 1;

    new->name = (char*)safe_malloc(sizeof(char) * (strlen(name) + 1));
    strcpy(new->name, name);

    new->instructions = (char*)safe_malloc(sizeof(char) * (strlen(instructions) + 1));
    strcpy(new->instructions, instructions);

    new->ingredients = ingredientList;
    new->next = NULL;

    if (start == NULL) return new;

    Recipe* temp = start;

    while (temp->next != NULL) temp = temp->next;

    new->index = temp->index + 1;
    temp->next = new;

    return start;
}

void displayRecipeNames(Recipe* start) {
    if (start == NULL) return;

    Recipe* temp = start;

    while (temp != NULL) {
        fprintf(stdout, "[%d] %s\n", temp->index, temp->name);
        temp = temp->next;
    }
}

void prettyPrintRecipe(Recipe* recipe) {
    if (recipe == NULL) {
        printf("Could not find recipe with given index.");
        return;
    }

    printf("==========================\n");
    printf("%s\n", recipe->name);
    printf("==========================\n");
    prettyPrintIngredients(recipe->ingredients);
    printf("Instructions:\n");
    printf("%s", recipe->instructions);
    printf("==========================\n");
}

void freeRecipe(Recipe* start) {
    if (start->next == NULL) return;

    freeRecipe(start->next);
    freeIngredient(start->ingredients);
    free(start->name);
    free(start->instructions);
    free(start);
}

Recipe* getRecipeByIndex(Recipe* start, unsigned int index) {
    Recipe* temp = start;

    while (temp != NULL) {
        if (temp->index == index) return temp;
        temp = temp->next;
    }

    return temp;
}

Recipe* readRecipe() {
    char buffer[2000];
    Ingredient* ingredients = NULL;
    Recipe* new = NULL;

    while (1) {
        fprintf(stdout, "Please enter the name of the recipe or (q) to quit:\n");

        if (fgets(buffer, sizeof(buffer), stdin) == NULL || strcmp(buffer, "\n") == 0) {
            fprintf(stdout, "Please enter a valid and non-empty name.\n");
            continue;
        }

        if (strcmp(buffer, "q\n") == 0) break;

        int nameLength = (int)strlen(buffer);
        char name[nameLength];
        buffer[strcspn(buffer, "\n")] = '\0';
        strcpy(name, buffer);

        fprintf(stdout, "Enter the amount of ingredients:\n");

        int ingredientAmount = 0;

        if (fgets(buffer, sizeof(buffer), stdin) == NULL || strcmp(buffer, "\n") == 0 || strcmp(buffer, "0\n") == 0) {
            fprintf(stdout, "Must specify at least 1 ingredient.\n");
            continue;
        }

        ingredientAmount = (int)strtoul(buffer, NULL, 10);

        if (ingredientAmount <= 0) {
            fprintf(stdout, "Must specify an amount >= 0.\n");
            continue;
        }

        printf("%d", ingredientAmount);

        ingredients = readIngredients(ingredientAmount);

        if (ingredients == NULL) return NULL;

        fprintf(stdout, "Please enter the cooking instructions:\n");

        if (fgets(buffer, sizeof(buffer), stdin) == NULL || strcmp(buffer, "\n") == 0) {
            fprintf(stdout, "Please enter non-empty instructions.\n");
            freeIngredient(ingredients);
            continue;
        }

        int instructionSize = (int)strlen(buffer);
        char instructions[instructionSize];
        strcpy(instructions, buffer);

        new = (Recipe*)safe_malloc(sizeof(Recipe));

        new->instructions = (char*)safe_malloc(sizeof(char) * instructionSize);
        strcpy(new->instructions, instructions);

        new->name = (char*)safe_malloc(sizeof(char) * nameLength);
        strcpy(new ->name, name);

        new->ingredients = ingredients;
        break;
    }

    return new;
}

main.c

#include <stdio.h>
#include <string.h>
#include <getopt.h>
#include "FileManager.h"

#define BUFF_SIZE 2000
#define EDIT "e\n"
#define ADD "a\n"
#define QUIT "q\n"

int main(int argc, char* argv[]) {

    int ch;

    while ((ch = getopt(argc, argv, "f")) != -1)
    {
        if (ch != 'f') {
            fprintf(stderr, "Invalid options defined.\n");
            return EXIT_FAILURE;
        }
    }

    if (optind != argc-1) {
        printf("Invalid parameter count.\n");
        return EXIT_FAILURE;
    }

    Recipe* recipes = parseRecipes(argv[optind]);

    if (recipes == NULL) {
        fprintf(stderr, "Error while parsing file.\nPlease check if the provided path is correct.\n");
        return EXIT_FAILURE;
    }

    char buffer[BUFF_SIZE];
    int recipeIndex = 0;

    displayRecipeNames(recipes);

    while (1) {
        fprintf(stdout, "Please select the number of the recipe to display, (e) to edit, (a) to add or (q) to quit:\n");

        if (fgets(buffer, BUFF_SIZE, stdin) != NULL) {
            if (strcmp(buffer, ADD) == 0) {
                Recipe* new = readRecipe();

                if (new == NULL) return EXIT_SUCCESS;

                Recipe* temp = recipes;
                while (temp->next != NULL) temp = temp->next;

                new->index = temp->index + 1;
                temp->next = new;

                writeToFile(recipes, argv[optind]);
                displayRecipeNames(recipes);
            } else if (strcmp(buffer, EDIT) == 0) {
                int recipeIndex = 0;

                fprintf(stdout, "Please the index of the recipe to edit:\n");
                if (fgets(buffer, BUFF_SIZE, stdin) == NULL || strcmp(buffer, "\n") == 0) {
                    fprintf(stderr, "Invalid input.");
                    continue;
                }

                recipeIndex = (int)strtol(buffer, NULL, 10);

                if (recipeIndex <= 0) {
                    fprintf(stderr, "Invalid input.");
                    continue;
                }

                Recipe* previous = getRecipeByIndex(recipes, recipeIndex == 1 ? recipeIndex : recipeIndex - 1);

                if (previous == NULL) {
                    fprintf(stdout, "Recipe at given index could not be found.\n");
                    continue;
                }

                Recipe* new = readRecipe();

                if (new == NULL) return EXIT_SUCCESS;

                if (previous->next == NULL) {
                    previous->next = new;
                    new->index = previous->index + 1;
                }
                else {
                    Recipe* old = previous->next;
                    previous->next = new;
                    new->next = old->next == NULL ? NULL : old->next;
                    new->index = old->index;
                    free(old);
                }

                writeToFile(recipes, argv[optind]);
                displayRecipeNames(recipes);
            } else if (strcmp(buffer, QUIT) == 0) {
                break;
            } else if ((recipeIndex = (int)strtol(buffer, NULL, 10)) > 0) {
                Recipe* recipe = getRecipeByIndex(recipes, recipeIndex);

                if (recipe == NULL) {
                    fprintf(stdout, "Recipe with index %d not found.\n", recipeIndex);
                    continue;
                }

                prettyPrintRecipe(recipe);
                displayRecipeNames(recipes);
            } else {
                fprintf(stderr, "Unknown command.\n");
            }
        }
    }

    freeRecipe(recipes);

    return EXIT_SUCCESS;
}
\$\endgroup\$
  • \$\begingroup\$ Your code looks perfect. It's confusing though that in the title of your question you promise to show us a recipe manager, and all we can see is a simple malloc wrapper. Your project is small enough that you can post all its files for review here. \$\endgroup\$ – Roland Illig Jan 19 at 8:21
  • 2
    \$\begingroup\$ I didn't want to pollute the question with files, that's why I linked the repository. But you're right. I added the other .c files to the question and would love to hear your opinion on the general structure of the program. \$\endgroup\$ – srole Jan 19 at 9:32
4
\$\begingroup\$

Recovery from Errors

The safe_malloc() function looks good, however, you could use setjmp and longjmp rather than exit(EXIT_FAILURE) to attempt to recover enough to clean up after errors occur and to only exit the program from main. According to an answer on this stackoverflow question they can also be used for co-routines. The additional information that exceptions provide for a fuller recovery is not present.

Portability

While the function getopt() is part of the POSIX standard, it is not part of the C99 programming standard, and will not be portable to all systems. It might be good to add this to the Utilities files. getopt() also seems to be implementing a global variable optind which is generally considered a bad programming practice.

Another function in some C libraries that is not standard that you may want to add to the Utilities files is char* strdup(char* Original) which could be used to replace the following blocks of code in `Recipe.c:

        int nameLength = (int)strlen(buffer);
        char name[nameLength];
        buffer[strcspn(buffer, "\n")] = '\0';
        strcpy(name, buffer);

        int instructionSize = (int)strlen(buffer);
        char instructions[instructionSize];
        strcpy(instructions, buffer);

Please note that the above code does not compile in a strict C compiler because arrays declared with a variable used as the length are not allowed. There is also an inherent bug in the above code because not enough memory will be allocated for the null terminator.

char *strdup(char *Original)
{
    size_t duplicate_size = strlen(Original) + 1;   // Allocate for the null terminator as well.
    char *duplicate = (duplicate_size > 1) ? safe_malloc(duplicate_size) : NULL;

    if (duplicate != NULL)
    {
        strcpy(duplicate, Original);
    }

    return duplicate;
}

The strdup() function will apparently be included in future C programming standards.

Missing Header Includes

The file main.c is missing the includes:
- #include <stdlib.h>
- #include "Recipe.h"

It really shouldn't compile.

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states that

every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

The SRP is one of the 5 principles in SOLID programming.

The while (1) in main() should be in its own function and should probably be broken up into multiple functions.

The functions Recipe* readRecipe() and Recipe* parseRecipes(const char* path) are also too complex. Generally any function that does not fit into one screen of an editor or IDE is too complex and should be broken up into multiple functions. I've had managers that insisted that any function larger than 10 lines to too complex, but I disagree with that.

Algorithm

Since recipes is a linked list it might be better to implement a full set of linked list operations such as create_recipe, insert_recipe, append_recipe, find_recipe and delete_recipe so that the program is easier to expand or modify. It might also be better to define two separate data structures: one for linked lists and one for recipes. The one for linked lists could be defined as:

typedef  struct node
{
    Recipe *data;
    struct node* next;
} Node;

This would allow the separation of the processing of the linked list from the processing of the recipe. The linked list operators above would then be create_node(Recipe* data), insert_node(Node *a_node), find_node() and delete_node().

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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