1
\$\begingroup\$

@Serge Bellesta suggested that once my program works correctly, I should consider posting it in Code Review to get interesting comments on it.

Problem statement(pdf): I would like my code to be more effiicient and guaranteed not to have some bad coding, e.g., when I press "enter" without typing in anything during the second question, it goes back to the question, but skips to the next iteration. I think(?) I set it back to the previous iteration, but no cigar:

for (i = 0; i < total_shops; i++)   {

printf("\nYou are at shop #%d.\n", i+1);

printf("\nHow many ingredients are needed at shop #%d?\n\nInput Specifications: Please type a positive integer and press the 'Enter' or the 'return' key when finished.\n", i+1);

fgets(line, sizeof(line), stdin);
line[strlen(line)-1] = '\0';

int sscanf_result = sscanf(line, "%d", &quantity_ingredients);

if ((sscanf_result == 0) | (sscanf_result == EOF)) {
printf ("\nInput Error: Please carefully read the input specifications that are provided after each question prompt and then try again.\n\n");
--i;
continue;
}

Nevertheless, the program works 100% if the user doesn't make any errors :)

This is the program in its entirety:

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

int main(void) {

int i, j, k, n;

char line[128]; /* the input line */

int total_shops; /* declare a variable for the total number of shops that will be visited */
total_shops = 0;

int quantity_ingredients; /* declare a variable for the quantity of ingredients that will be purchased */
quantity_ingredients = 0;

double **cost_ingredients_ptr; /* declare an 2D array for the cost of each ingredient purchased */

cost_ingredients_ptr = (double**)calloc(128, sizeof(double*));

for (i = 0; i < 128; i++)    {
cost_ingredients_ptr[i] = (double*)calloc(128, sizeof(double));
}

double *total_cost_ingredients_ptr;
total_cost_ingredients_ptr = (double*)calloc(128, sizeof(double));

int location_cheapest_order = 1;

while (total_shops == 0) {

printf("How many shops will be visited?\n\nInput Specifications: Please type a positive integer and press the 'Enter' or the 'return' key when finished.\n");

fgets(line, sizeof(line), stdin);
line[strlen(line)-1] = '\0';    /* trim off last character */

int sscanf_result = sscanf(line, "%d", &total_shops);

if ( (sscanf_result == 0) | (sscanf_result == EOF) ) { /* either a non-integer entered or an end-of-line */

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

for (i = 0; i < total_shops; i++)   {

printf("\nYou are at shop #%d.\n", i+1);

printf("\nHow many ingredients are needed at shop #%d?\n\nInput Specifications: Please type a positive integer and press the 'Enter' or the 'return' key when finished.\n", i+1);

fgets(line, sizeof(line), stdin);
line[strlen(line)-1] = '\0';

int sscanf_result = sscanf(line, "%d", &quantity_ingredients);

if ((sscanf_result == 0) | (sscanf_result == EOF)) {
printf ("\nInput Error: Please carefully read the input specifications that are provided after each question prompt and then try again.\n\n");
--i;
continue;
}

int offset;

for (j = 0; j < quantity_ingredients; j++) {

printf("\nWhat is the cost of ingredient #%d?\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.", j+1);

fgets(line, sizeof(line), stdin);
line[strlen(line)-1] = '\0';

if (sscanf(line, "%lf%n", &cost_ingredients_ptr[i][j], &offset) != 1) {
return 1;
}

total_cost_ingredients_ptr[i] += cost_ingredients_ptr[i][j];    /* computation of the total cost of ingredients*/
}

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

if (i == total_shops-1) {

double cheapest_order;

cheapest_order = total_cost_ingredients_ptr[0];

for (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);
}
}
}
for (n = 0; n < 128; n++){
free(cost_ingredients_ptr[n]);  /* releases allocated block on heap */
}
free(cost_ingredients_ptr);
free(total_cost_ingredients_ptr);
return 0;
}
\$\endgroup\$
3
  • \$\begingroup\$ Please provide a description of the problem (i.e. a problem statement) to help us understand what you're trying to do. I went looking for a question asked on Stack Overflow since you referenced a user recommending Code Review, but all I could find was this one which seems to be about a different problem. \$\endgroup\$
    – Null
    Commented Oct 3, 2018 at 14:42
  • 1
    \$\begingroup\$ Please add some description about your code( what 's purpose of the program, how to you expected it work,...). In addition, your source code is not follow coding convention. You can refer this: google.github.io/styleguide/… \$\endgroup\$
    – Nhan Phan
    Commented Oct 3, 2018 at 16:02
  • \$\begingroup\$ @NhanPhan I added a pdf of the problem statement. \$\endgroup\$ Commented Oct 4, 2018 at 9:58

1 Answer 1

1
\$\begingroup\$

Code formatting

I recommend you use an "indent" tool to align your code consistently with its brace level. That makes it much easier to read and to reason about.


Variable declarations

Back in pre-Standard C (and in the first version of the Standard), it used to be necessary to declare variables at the beginning of their enclosing block like this:

int i, j, k, n;

char line[128]; /* the input line */

That's not necessary in modern C; it's much better to declare local variables where they are first initialised (this helps avoid the error of omitting the initialisation, for one thing).

Also, some of these can be moved into smaller scopes. In particular, the loop index variables can be local to the loops themselves, like this:

for (int i = 0;  i < total_shops;  i++) {

Memory allocation

If we've correctly included <stdlib.h>, then malloc(), calloc() and realloc() are known as functions returning void*. Such a pointer can be assigned to any pointer type without needing a cast.

It's probably not a good idea to use calloc() to allocate an array of pointers, or of floating-point numbers. That's because it initialises the memory to all bytes zero, which is at best implementation-defined when interpreted as a pointer or double. Instead, use plain malloc() and initialise the contents programmatically.

It's safest to use the pattern p = malloc(sizeof *p) when allocating an object (or count * sizeof *p for an array), so that it's obvious and automatic that the allocated size corresponds to the type as which it will be used.

Then our allocations look more like this:

double **cost_ingredients_ptr = malloc(128 * sizeof *cost_ingredients_ptr);
if (!cost_ingredients_ptr) {
    fprintf(stderr, "Memory allocation failure!\n");
    exit(1);
}

We must check that the allocation function didn't return a null pointer before trying to use the memory - failing to do so is a common mistake that leads to Undefined Behaviour (commonly a program crash, but potentially any action is possible).


Magic numbers

What's the significance of 128 in the size of line? Does it need to be the same as the number of cost ingredients, or is that just coincidence? If we give names to these quantities, it can help the reader understand the code better, and give a single point of change if the number needs adjusting.


Input validation

It's good that there's an attempt at input validation:

    int sscanf_result = sscanf(line, "%d", &total_shops);

    if (sscanf_result == 0 | sscanf_result == EOF) {

However, we've missed one important test - we'll accept a negative number just as happily as a valid, positive number. We can also make the test of sscanf_result simpler - we don't care whether we got an empty line or an invalid one, so we only require that the result is exactly 1:

    if (sscanf_result != 1 || total_shops <= 0) {

Notice also that we have a big block of repeated code. This is a good sign that we'd benefit by creating a function we can call from the two different places:

#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[128];
        fgets(line, sizeof line , stdin);
        /* no need to trim final newline - sscanf() skips whitespace */
        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");
    }
}

Then we get something as simple and expressive as:

int total_shops = read_positive_int("How many shops will be visited?");

Note that if the user types a line longer than 128 characters, the excess will be considered to be a separate line, and may be treated as valid input if it begins with a number.

We might choose instead to directly scanf() the input, like this:

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 (scanf("%d", &n) != 1 || n <= 0) {
        /* consume input to next newline */
        int c;
        while ((c = getchar()) != EOF && c != '\n') ;  /* empty loop */
        if (c == EOF) {
            fprintf(stderr, "Input failure!\n");
            exit(1);
        }
        printf ("\nInput Error: Please carefully read the input specifications that are provided after each question prompt and then try again.\n\n");
    }
}

We can make a similar function for entering the currency amounts.


Don't use floating-point for money

Binary fractions can't exactly represent 0.01, so we'll get rounding errors when adding. Instead, we should read the units and hundredths separately, and combine them into integer number of pennies/cents/centimes/etc for arithmetic, and separate them again for display. We might use a format like "%d.%2d" to help us read the values, and cost / 100 and cost % 100 for printing.


Unnecessary storage

We allocate a lot of memory, but don't actually need to store most of it. All we need is a running total for the current shop, and a record of which shop was the least expensive so far, and how much its total was. So we can re-think the whole strategy to be much leaner in operation, storing just those values rather than everything that was entered, and never needing to allocate and free any dynamic storage at all.

\$\endgroup\$
6
  • \$\begingroup\$ Thank you for your input. I'm going to work on improving my code based on your suggestions, but I did want to make a comment about Magic Numbers. I originally wanted to do something like cost_ingredients[total_shops][quantity_ingredients] since that would yield a matrix exactly the dimensions necessary, but I couldn't figure out how. I chose 128 for two reasons: #1 Powers of 2 because I read something about shifting instead of multiplying being computationally less expensive, and #2 I wanted to allow enough characters in case the ingredient was in the XXX,XXX,XXX.XX price range. \$\endgroup\$ Commented Oct 3, 2018 at 19:27
  • \$\begingroup\$ I also wanted to use realloc in case the user went to more than 128 stores, but I also couldn't figure out how to use realloc with a 2D array, free() it afterwards, and also reset it to NULL to prevent a dangling pointer. \$\endgroup\$ Commented Oct 3, 2018 at 19:36
  • \$\begingroup\$ if ( (i%128)-127 == 0 ) { cost_ingredients_ptr = realloc(cost_ingredients_ptr, i + 128); memset(cost_ingredients_ptr+i, 0, 128); /* memset extra elements to 0 */ } \$\endgroup\$ Commented Oct 3, 2018 at 19:41
  • \$\begingroup\$ You would be able to use realloc() (with care; dealing with failures can be tricky), but it's probably easier to build a linked list, with each store pointing to the next (or previous). In any case, it's moot: as my last paragraph says, we don't need any of that storage. \$\endgroup\$ Commented Oct 4, 2018 at 7:39
  • \$\begingroup\$ I posted some new code. Made some improvements based on your suggestions, still working on removing dynamically allocated space and not using floating-point for money. \$\endgroup\$ Commented Oct 4, 2018 at 10:01

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.