Cfrtjryk

I am a newbie in a programming, and I just randomly chose a task for training from some group on Facebook. The task is to calculate the cost of cement for a construction project. The input is the number of pounds of cement required (guaranteed not to be a multiple of 120). The store sells cement in 120-pound bags, each costing $45.

Example input: 295.8

Output: 135

#include <stdio.h>

#include <stdlib.h>



#define PRICE 45

#define CAPACITY 120

#define MAXDIGITS 5



int sum(int);



int main(int argc, char *argv)

{

    int val = 0;

    char inp[MAXDIGITS];



    if ((argc > 1) && (argv[1] > 0))

        val = strtol(argv[1], NULL, 0);

    else

    {

        do 

        {

            printf("Please input the value of cement: ");

            scanf("%s", inp);

            val = strtol(inp, NULL, 0);

        }

        while (!val);

    }



    if (val)

        printf("Money you need: %dn", sum(val));



    return 0;

}



int sum(int need)

{

    int mon = PRICE;

    int n = CAPACITY;



    while (n < need)

    {

        n += CAPACITY;

        mon += PRICE;

    }



    return mon;

}

I'm interested in code style, rational memory usage, etc.

Welcome on Code Review

Review

#define PRICE 45

#define CAPACITY 120

#define MAXDIGITS 5

• You define PRICE and CAPATITY as integers, but a price can have cents, and a bag can maybe contains more than a plain amount in pounds, maybe some ounces more. So you should use decimals here.

if ((argc > 1) && (argv[1] > 0))

• Don't compare a char* to an int. This check doesn't insure that argv[1] is a valid number.


• If the parameter isn't what you want, you can either print the usage and quit, or silently continue and ask for input.

val = strtol(argv[1], NULL, 0);

• You don't validate the program argument. strtol can silently fail and return "0". In this case, a good option would be to ask for a good input.


• 
  

  You parse the string to an unsigned integer; but the asked task stand asking a decimal number (and show "295.8" in the example). You could use strtod or atof but since you are a conscientious programmer, you want to check for validity.

  
  
  

  So the combo scanf/sscanf (with "%f" or "%lf")` are the solution.

  
  

do 

{

    printf("Please input the value of cement: ");

• Try to put a nbefore your request. It will make the output more clear. Otherwise, it will print on the same line as last output.

scanf("%s", inp);

val = strtol(inp, NULL, 0);

• As above, use scanf("%lf", ...) instead (shorter, and you can check for errors)

int sum(int need)

Once what i said above is fixed (price and capacity as double) your function work fine. However, you can simplify it, by doing the computation in only one line (which can be simplified even more, with libmath).

Here's my corrected version:

#include <stdio.h>

#include <stdlib.h>



#define PRICE 45.

#define CAPACITY 120.



int sum(int);



int main(int argc, char *argv)

{

    double need = 0.;



    if (argc < 2 || sscanf(argv[1], "%lf", &need) != 1) {

        while (printf("nPlease input the amount of cement you need (e.g. 295.8): ") && scanf("%lf", &need) != 1) {

            for(int c = 0; c != 'n' && c != EOF; c = getchar());

        } 

    }

    printf("For %.02lf pounds you need: $%.02lf!n", need, PRICE * ((int)((int)(need / CAPACITY) * CAPACITY < need) + (int)(need / CAPACITY)));



    // or with "#include <math.h>" and the compiler/linker flag "-lm"

    //printf("For %.02lf pounds you need: $%.02lf!n", need, PRICE * ceil(need / CAPACITY));

    return 0;

}

Buffer overflow

Consider what entering "295.8" does.

#define MAXDIGITS 5

char inp[MAXDIGITS];

printf("Please input the value of cement: ");

scanf("%s", inp);

Code used the dangerous scanf("%s", inp); with no width limits. scanf("%s", inp); being ignorant of the size of inp, stored the 5 read characters and the appended null character.

This results in undefined behavior (UB). Code may work as expected today and fail is strange ways tomorrow.

I recommend to only use fgets() for user input. After reading a line of user input, it is saved as a string. Then parse the string.

No need for such a tight buffer size. Recommend twice the expected max width needed.

#define BUFFER_SIZE (2*MAXDIGITS + 1)

char buffer[BUFFER_SIZE];



printf("Please input the value of cement: ");

fflush(stdout);  // Insure output is seen before reading



fgets(buffer, sizeof buffer, stdin);

double val = strtod(buffer, NULL, 0);