Find the cheapest order placed out of all of the stores visited - follow-up #1

Question

Made some adjustments to my original program. Here's a link to it: original. Also, here's a link to the problem statement: problem statement.

The assignment is past due; The code I turned in is stupid simple. Which is great, but I didn't learn anything, so I am testing the many different ways to do things in C, and hoping some will have some hints, tips, tricks, advice, suggestions, etc.

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

int read_positive_int(const char* prompt)
{
    printf("%s\n\nInput Specifications: Please type a positive integer and press the 'Enter' or the 'return' key when finished.\n", prompt);

    int n;
    while (true)
    {
        char line[1024];    /* the input line */
        fgets(line, sizeof(line), stdin);

        int sscanf_result = sscanf(line, "%d", &n);
        if (sscanf_result == 1 && n > 0)
            {
                return n;
            }

        puts("\nInput Error: Please carefully read the input specifications that are provided after each question prompt and then try again.\n");
    }
}

float read_real_positive_float(const char* prompt)
{
    printf("%s\n\nInput Specifications: Please type a real number in currency format, i.e., XXX.XX, and press the 'Enter' or the 'return' key when finished.\n", prompt);

    float n;
    while (true)
    {
        char line[1024];
        fgets(line, sizeof(line), stdin);

        int sscanf_result = sscanf(line, "%f", &n);
        if (sscanf_result == 1 && n > 0 )
            {
                return n;
            }
        puts("\nInput Error: Please carefully read the input specifications that are provided after each question prompt and then try again.\n");
    }
}

int main(void)
{
    int total_shops = read_positive_int("How many shops will be visited?");
    float **cost_ingredients_ptr = malloc(total_shops * sizeof(float*)); /* allocate an array of pointers */

    if (!cost_ingredients_ptr)
    {
        fprintf(stderr, "Memory allocation failure!\n");
        exit(1);
    }

    float **total_cost_ingredients_ptr = malloc(total_shops * sizeof(float*));
    memset(total_cost_ingredients_ptr, 0, total_shops * sizeof(float));

    if (!total_cost_ingredients_ptr)
    {
        fprintf(stderr, "Memory allocation failure!\n");
        exit(1);
    }

    for (int i = 0; i < total_shops; i++)
    {
        printf("\nYou are at shop #%d.\n\n", i+1);
        int quantity_ingredients = read_positive_int("How many ingredients are needed?");

        cost_ingredients_ptr[i] = malloc(quantity_ingredients * sizeof(float));

        if (!cost_ingredients_ptr[i])
            {
                fprintf(stderr, "Memory allocation failure!\n");
                exit(1);
            }

        total_cost_ingredients_ptr[i] = malloc(sizeof(float));

        if (!total_cost_ingredients_ptr)
            {
                fprintf(stderr, "Memory allocation failure!\n");
                exit(1);
            }

        for (int j = 0; j < quantity_ingredients; j++)
            {
                printf("\nWhat is the cost of ingredient #%d", j+1);
                cost_ingredients_ptr[i][j] = read_real_positive_float("?");
                *total_cost_ingredients_ptr[i] += cost_ingredients_ptr[i][j];
            }

        printf("\nThe total cost at shop #%d is $%0.2f.\n", i+1, *total_cost_ingredients_ptr[i]);

        if (i == total_shops-1)
        {
            float cheapest_order = *total_cost_ingredients_ptr[0];
            int location_cheapest_order = 1;

            for (int k = 1; k < total_shops; k++)
            {
                if (*total_cost_ingredients_ptr[k] < cheapest_order)
                {
                    cheapest_order = *total_cost_ingredients_ptr[k];
                    location_cheapest_order = k + 1;
                }
                printf("\nThe cheapest order placed was at shop #%d, and the total cost of the order placed was $%0.2f.\n", location_cheapest_order, cheapest_order);
            }
        }
    }
free(cost_ingredients_ptr);
free(total_cost_ingredients_ptr);
cost_ingredients_ptr = NULL;
total_cost_ingredients_ptr = NULL;
return 0;
}

Answer 1

Don't be overly verbose

Your prompts are very verbose, and you use a lot of newlines. Keep it simple. If you ask "how many", then people naturally expect to enter a natural number. If you ask for "cost", then people will naturally assume they can enter some value with two digits after the comma.

If you prompt something, replace the newline after the prompt with a space, then the cursor will be after the question.

You also don't need to read a whole line into a buffer, and then use sscanf() on the buffer, you can directly use scanf() to read a value from the standard input. I would just write:

printf("How many shops will be visited? ");
int total_shops;
if (scanf("%d", &total_shops) != 1) {
    fprintf(stderr, "Invalid input!\n");
    return 1;
}

Also, try to use \n only at one end of format strings, instead of both at the start and the end.

Don't use the _ptr postfix

I would avoid using prefixes or postfixes that describe the type of a variable. It is usually not necessary.

Use structs to organize your data

You have a number of shops, and each shop has a number of ingredients. Instead of making total_cost_ingredients_ptr a pointer to pointer, it is better to define a struct that represents a shop, and then to create an array out of the shops, like so:

struct shop {
    int num_ingredients;
    float *ingredient_costs;
};

struct shop *shops = calloc(num_shops, sizeof(*shop));

for (int i = 0; i < num_shops; i++) {
     int num_ingredients = ...; /* Read number of ingredients */

     shops[i].num_ingredients = num_ingredients;
     shops[i].ingredient_costs = calloc(num_ingredients, sizeof(*shops[i].ingredient_cost));
     ...
}

Later on you can then refer to ingredient number j in shop i as: shops[i].ingredient_costs[j].

Note that I used calloc() here instead of malloc(). It is slightly easier to use the former to allocate arrays, and it will also pre-zero the allocated memory for you.

Move actions to be done after the last shop data is read to after the loop

Whenever you see this pattern:

for (int i = 0; i < num; i++) {
    ...
    if (i == num - 1)
        do_something();
}

Just move that last part out of the loop:

for (int i = 0; i < num; i++) {
    ...
}

do_something();

Move long sections of code into functions

You created read_positive_int() and read_real_positive_float() for relatively trivial code, but you forgot to put the code to read all ingredients for a shop into its own function. You can structure the code like so:

void read_shop_ingredients(struct shop *shop) {
    ...
}

int main() {
    struct shop *shops = ...;
    ...
    for (int i = 0; i < num_shops; i++) {
         read_shop_ingredients(&shops[i]);
    }
}

Don't store temporary data longer than necessary

You are storing the cost of each individual ingredient in const_ingredients, but you are actually only interested in the sum of the ingredients for each store. So don't store the individiual costs, but while reading the costs, add them immediately to the total_costs_ingredients.

Don't use a 2D array for 1D data

You basically copy&pasted the code for costs_ingredients to total_costs_ingredients, creating a 2D array where one of the dimensions is just 1 item big. That's not very efficient. Also, if you use the struct approach, it would've been obvious right away.

So with this in mind, the struct can be changed to:

struct shop {
    float total_ingredient_costs;
};

And a struct with just one element is a bit overkill; you could have gotten away with allocating just a single 1D array in your code:

float *total_ingredient_costs = calloc(num_shops, sizeof(*total_ingredient_costs));

Use return instead of exit() if possible

There is no need to call exit() inside main(), just use return 1 to exit with an error code.

Don't clear local variables at the end of a function

While it is certainly good to call free() for every piece of memory you allocated with malloc() and calloc(), there is no need to set the pointers to NULL here.

Answer 2

Avoid size mistakes

What is wrong with memset() in the following OP's code?

float **total_cost_ingredients_ptr = malloc(total_shops * sizeof(float*));
memset(total_cost_ingredients_ptr, 0, total_shops * sizeof(float));

Mouse over for the answer

sizeof(float) is not certainly the size needed. sizeof(float*) would have been OK.

---

When allocating and referring to the size of an object or a pointed to object, prefer to use the sizeof the (pointed) object and not its type.

It is easier to code right, review and maintain.

// float **cost_ingredients_ptr = malloc(total_shops * sizeof(float*));
float **cost_ingredients_ptr = malloc(total_shops * sizeof *cost_ingredients_ptr);

// memset(total_cost_ingredients_ptr, 0, total_shops * sizeof(float));
memset(total_cost_ingredients_ptr, 0, total_shops * sizeof *total_cost_ingredients_ptr);

When calculating size, prefer to start with the size_t argument.

size_t * int has the same effect as int * size_t.

size_t * int * int is not as int * int * size_t. The later performs int * int which may overflow wheres size_t * int * int may not.

Recommend

// float **cost_ingredients_ptr = malloc(total_shops * sizeof(float*));
float **cost_ingredients_ptr = malloc(sizeof *cost_ingredients_ptr * total_shops);

---

If code is zero-ing allocated memory, consider calloc() advantages:

//float **total_cost_ingredients_ptr = malloc(total_shops * sizeof(float*));
//memset(total_cost_ingredients_ptr, 0, total_shops * sizeof(float));

float **total_cost_ingredients_ptr = calloc(total_shops,sizeof *total_cost_ingredients_ptr);

1) Cleaner DRY code.

2) Typically as fast or faster.

3) No potential NULL deference. - Still following code should check for a NULL return.

4) On select systems, calloc() can allocate more than SIZE_MAX total bytes whereas malloc() is limited.

5) A good calloc(n, size) will detect products of n * size greater the SIZE_MAX and allocate accordingly. Whereas malloc(n * size) simply overflows the product.