1
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Please copy the problem statement into your question instead of linking to an external site. \$\endgroup\$
    – G. Sliepen
    Oct 4, 2018 at 17:43

2 Answers 2

3
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ Some of these points were made in my review of the first iteration (particularly, "Don't store temporary data longer than necessary") but don't seem to have been actioned for this revision. :-( I did argue against calloc() there, so we've given conflicting advice - as we've both justified the advice, the reader can at least make an informed choice! :-) \$\endgroup\$ Oct 4, 2018 at 20:01
  • \$\begingroup\$ @TobySpeight: I hadn't read your review. Indeed, if zeroing the memory is not necessary, then calloc() is probably not the best choice. I wrote that because the code in this review did a memset() right after a malloc(). There's also the added benefit of calloc() that it prevents overflow of 32-bit multiplication if one is not careful (but then again, if one of the sides of the multiplication is a sizeof then it doesn't matter). \$\endgroup\$
    – G. Sliepen
    Oct 4, 2018 at 20:19
  • \$\begingroup\$ @TobySpeight I appreciated your answer very much. In my last comment to you, I did mention I was going to work on not using memory allocation and floating point for money. I wanted to focus on implementing functions as you suggested and eliminating the need for magic numbers.I just edited my answer to reflect some changes I made, namely, eliminating memory allocation and creating arrays exactly the length and size necessary by using the user's input to declare them. I think you'll be happy with the improvement! \$\endgroup\$ Oct 5, 2018 at 11:28
  • \$\begingroup\$ @G.Sliepen Thank you for your suggestions! I made so many changes based on your help and the code is much different now. There are some things I still need to work on, but I think you'll like the most recent edit to my question \$\endgroup\$ Oct 5, 2018 at 11:32
1
\$\begingroup\$

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.

\$\endgroup\$
10
  • \$\begingroup\$ memset() is technically wrong for nulling pointers, anyway - and these values aren't used before they are next assigned, so the memset() is pointless, too. (sorry, no pun intended) \$\endgroup\$ Oct 5, 2018 at 12:21
  • 1
    \$\begingroup\$ Is advantage 4 really true? It seems a contradiction given size_t can represent the size of any object, if calloc() could return a pointer to an object larger than SIZE_MAX. \$\endgroup\$ Oct 5, 2018 at 12:25
  • \$\begingroup\$ @TobySpeight Allocated memory is not an object. Common flat memory models do not provide double *p = calloc(SIZE_MAX - 10, sizeof *p); with a non-NULL result yet C allows it. \$\endgroup\$ Oct 5, 2018 at 12:49
  • \$\begingroup\$ @TobySpeight I was not suggesting memset() for nulling pointers, but for zeroing memory - which calloc() is good for. Even if later code first assigns, calloc() does follow the RAII principle - especially useful for debugging. Yet as you say malloc() would suffice without the memset() here. \$\endgroup\$ Oct 5, 2018 at 12:55
  • \$\begingroup\$ Oh, absolutely - if malloc() needs to be followed by memset(), then calloc() is a better choice. That's just not the case for this program. I think we do agree, really. (Have a good weekend - I'm about to leave soon). \$\endgroup\$ Oct 5, 2018 at 13:57

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.