6
\$\begingroup\$

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: %d\n", 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.

\$\endgroup\$
-2
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Agree with you. Only one small mark for your code. If it receive a characters instead of digits, it enters to infinite loop. I think there is no alternative to use additional variable to avoid that possibility. \$\endgroup\$ – Dmitry Khaletskiy Nov 25 '18 at 23:02
  • 1
    \$\begingroup\$ @DmitryKhaletskiy fixed \$\endgroup\$ – Calak Nov 25 '18 at 23:38
  • 3
    \$\begingroup\$ Sorry, but this code is severely lacking and definitely not something that should be used in place of the OP's much more readable code. The calculations badly need to be split over several lines. Arbitrary checking the return value of printf is fishy. C programming isn't about writing as few LOC as possible, if that comes at the expensive of readability. \$\endgroup\$ – Lundin Nov 27 '18 at 14:33
  • \$\begingroup\$ while (... scanf("%lf", &need) != 1) { is an infinite loop on end-of-file, input error. \$\endgroup\$ – chux - Reinstate Monica Nov 27 '18 at 18:33
  • \$\begingroup\$ (int) unnecessarily limits a floating-pint function to the int range - promoting a weak programing paradigm. Robust code would use rint(), round(), trunc() etc. instead. \$\endgroup\$ – chux - Reinstate Monica Nov 27 '18 at 18:36
2
\$\begingroup\$

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 in 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);
\$\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.