2
\$\begingroup\$

The code should enter an integer between 5-95 and calculate how many coins, coin values are 50, 20, 10 and 5.

My concern is the CalcChange function and whether that is acceptable C code.

#include <stdio.h>

#define TRUE 1
#define FALSE 0


void GetNum(int &amount)
{
        printf("Enter an integer between 5 and 95 inclusive:\n");
        scanf("%d%*c", &amount);

        return;
}

int Verify(int amount)
{
        if(amount <= 4 || amount >= 95)
                return FALSE;

        return(TRUE);
}

//Function calculates number of coins for each denomination
//and returns the remainder as the new value of amount
//until remainder is <= 4

int CalcChange(int &amount, int denomination)
{

        int change = amount / denomination;

        amount = amount % denomination;

        return(change);

}

//Instead of wasting the remainder, lets donate it to charity
void PrintResult(int fifty, int twenty, int ten, int five, int remainder)
{
        printf("\n%d 50c coins\n%d 20c coins\n%d 10c coins\n%d 5c coins.\n", fifty, twenty, ten, five);
        printf("\nThe remainder of %d cents will be donated to charity.\n", remainder);

        return;
}

int main()
{
        int a, change, fifty, twenty, ten, five, remainder;

        GetNum(a);

        Verify(a);

        if(Verify(a))
        {
                fifty = CalcChange(a, 50);
                twenty = CalcChange(a, 20);
                ten = CalcChange(a, 10);
                five = CalcChange(a, 5);
                remainder = CalcChange(a, 1);

                PrintResult(fifty, twenty, ten, five, remainder);
        }
        else
        {
                printf("\n%d is out of range!\n", a);
        }

        return(0);
}
\$\endgroup\$
  • 5
    \$\begingroup\$ void GetNum(int &amount) is not valid C. This looks like it might be C++ code, though. If that's the case, please change your tags accordingly. \$\endgroup\$ – Ben Steffan Apr 26 '18 at 14:28
  • \$\begingroup\$ Does your code work like you want it to? \$\endgroup\$ – Malachi Apr 26 '18 at 14:42
  • \$\begingroup\$ hi Ben it is C, however i am using pass by reference and our lecturer instructed us that to do this in C all we do is use the & \$\endgroup\$ – Sean Cornell Apr 26 '18 at 14:44
  • \$\begingroup\$ the code does work yes, but i am trying to learn to write good code and looking for feedback \$\endgroup\$ – Sean Cornell Apr 26 '18 at 14:48
  • \$\begingroup\$ @SeanCornell "using pass by reference and our lecturer instructed us that to do this in C" is suspicious. Review notes or understand lecturer is not instructing standard C. \$\endgroup\$ – chux Apr 26 '18 at 16:40
2
\$\begingroup\$

Invalid C code

OP’s C compiler may support various language extensions, but int &amount is not supported in standard C @Ben Steffan. This makes the code less portable and harder to review.

// void GetNum(int &amount)
// scanf("%d%*c", &amount);
void GetNum(int *amount)  // Let caller pass the address of `amount` instead
scanf("%d%*c", amount);

Lack of error checking/weak end-of-line consumption

scanf("%d%*c", &amount); does not check the return value from scanf(). Should the user enter non-numeric data, nothing is scanned, amount will not be changed and offending input remains in stdin. Alternative code;

int GetNum(int *amount) {
  printf("Enter an integer between 5 and 95 inclusive:\n");
  char buf[80];
  if (fgets(buf, sizeof buf, stdin)) return EOF;
  char ch;
  // Does input consists of one `int` and not trailing junk?
  if (sscanf(buf, "%d %c", amount, &ch) != 1) return 0; // Invalid input
  return 1; // success;
}

Unneeded ()

// return(change);
return change;

Consider standard true/false and bool.

// #define TRUE 1
// #define FALSE 0
// int Verify(int amount)
//   return FALSE;

#include <stdbool.h>
bool Verify(int amount)
  return false;

See also to use '#include ' in VS 2010 and We also added the new headers ..., stdbool.h,


Thought for a likely next assignment:

Consider how to make 60c change when the available coins are only 50c,20c,20c,20c using the below algortim.

fifty = CalcChange(a, 50, 1);
twenty = CalcChange(a, 20, 3);
ten = CalcChange(a, 10, 0);
\$\endgroup\$
  • \$\begingroup\$ I'm not aware of any C dialect that supports the C++ reference syntax. Again, I'm guessing that might actually be writing C++ (i.e. using a C++ compiler to compile his code, which is really close to C). Please prove me wrong, though. \$\endgroup\$ – Ben Steffan Apr 26 '18 at 16:51
  • \$\begingroup\$ @BenSteffan Agree OP is likely using C++, but if using C++ or some C with &, the code does not compile with a standard C compiler - and that is the key point. I worked with & as a C extension BITD. Unfamiliar if a C modern extensions exists or not. So my proof is lost. in some grey cells. C and C++ used to live in the same house amicably as parent - child, but they have moved to separate addresses and have ever decreasing commonality. \$\endgroup\$ – chux Apr 26 '18 at 17:01
  • \$\begingroup\$ I think // isn't C, too. even it is accepted by many compilers \$\endgroup\$ – miracle173 Apr 26 '18 at 17:45
  • \$\begingroup\$ @miracle173 Re: // isn't C. Since C99 (19 years ago) // has been an acceptable C comment. Many C compilers allowed it even before then. Certainly some coders still today use pre C99 compilers that do not. \$\endgroup\$ – chux Apr 26 '18 at 17:51
  • \$\begingroup\$ thank you to everyone for all your input. Clarification: i do have to save the file as .cpp so Ben you are probably right, i should have tagged it C++ \$\endgroup\$ – Sean Cornell Apr 26 '18 at 23:46
0
\$\begingroup\$

I cannot offer much C experience, but I think the code is very well structured. The output part is encapsulated in a own functions and also the other tasks.

  • The input isn't checked.
  • I think the call of the Verify function after the GetNum function in the mainfunction does not make sense.

    int main()
    {
        int a, change, fifty, twenty, ten, five, remainder;
    
        GetNum(a);
    
        Verify(a);
       ...
    

    I think with the appropriate compiler settings you should get a warning if you use a function and do not use its return value. So use appropriate compiler warnings and do not ignore them.

  • remainder=CalcChange(a, 1) does not make sense, too. This is equivalent to remainder=a

  • The use of constant for 5,10,20,50 may it make simpler to change the program and avoids the use of magic numbers. Of course the names of the variables should be changed, too, if you use constants.
  • Your program says "Enter an integer between 5 and 95 inclusive:\n", but when I input 95 it tells me 95 is out of range!
  • it is not a good idea to use the variable name fifty, twenty, ten and five.

            fifty = CalcChange(a, 50);
            twenty = CalcChange(a, 20);
            ten = CalcChange(a, 10);
            five = CalcChange(a, 5);
    

    looks ugly but

            coins50 = CalcChange(a, 50);
            coins20 = CalcChange(a, 20);
            coins10 = CalcChange(a, 10);
            coins5 = CalcChange(a, 5);
    

    looks nicer an is easier to understand, especially if you don't speak English. If you extend your program to handle a coin with value twothousandsevenhundredandseventyfive it looks even more ugly. But I think it is not a good idea that the variable names contain information of the variable names at all, but if you rewrite your program that it works for coins with values 100, 50, 25, 10 then you have to change the variable names, too. Otherwise your naming is absurd. So it is better to name them coin1, coin2, coin3, coin4 or similar, where coin1 stands for the highest coin, coin2 for the second highest coin an so on.

\$\endgroup\$
  • \$\begingroup\$ excellent suggestions thank you @miracle173 \$\endgroup\$ – Sean Cornell Apr 27 '18 at 4:45

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.