C++

Forum

 
I need helpful critique for school assignment.

mishappp (59)
I'm very new to c++ I started a few weeks ago at school, this is one of the project assignments. I was hoping for some input on what I did or what I should have done. Also any advice on making the code look aesthetically more pleasing. Thanks!

The program takes a random number from 0-99 and calculates how much change in the lowest form of coins you should receive.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
 
#include <iostream>
#include <iomanip>
#include <cstdlib>
#include <ctime>
 
using namespace std;
 
int main()
{
        unsigned change = time(0);
        int quarters = 0;
        int dime = 0;
        int nickel = 0;
        int penny = 0;
        int quartervalue = 0;
        int dimevalue = 0;
        int nickelvalue = 0;
        int pennyvalue = 0;
 
        // Get a random number from 0-100
        srand(change);
        change = change % 100;
 
 
        // If change is greater than or equal to one run the program
                if (change >= 1)
                {
 
        //If change is greater than or equal to 10 skip decimal fix
                 if (change >= 10)
                {
                        cout << "Change Due:" << setw(6) << "$0." << change << "\n";
                        cout << "Coin Dispenser will dispense:\n";
                }
 
        // Decimal fix for under 10 cents.
                  else
                {
                        cout << "Change Due:" << setw(6) << "$0.0" << change << "\n";
                }
 
 
        // If change is greater than or equal to 25, get quarters, adjust change, and get the quarter value.
                 if (change >= 25)
                {
                        quarters = change / 25;
                        change = change % 25;
                        quartervalue = quarters * 25;
 
                }
        // If change is greater than or equal to 10, get dimes, adjust change, and get the dime value.
 
                 if (change >= 10)
                {
                        dime = change / 10;
                        change = change % 10;
                        dimevalue = dime * 10;
                }
        // If change is greater than or equal to 5, get nickels, adjust change, and get the nickel value.
                 if (change >= 5)
                {
                        nickel = change / 5;
                        change = change % 5;
                        nickelvalue = nickel * 5;
                }
        // If change is less than or equal to 4, get pennies, get value.
             if (change <= 4)
                {
                        penny = change / 1;
                        pennyvalue = penny * 1;
                }
 
 
                if (quartervalue == 0)
                {
                        // If quartervalue is equal to 0, don't display.
                }
 
                else
                {
                        cout << " Quarters: " << quarters << " (" << "$0." << quartervalue << ")" << "\n";
                }
 
 
                if (dimevalue == 0)
                {
                        // If dimevalue is = to 0, don't display.
                }
 
                else
                {
                        cout << " Dimes:" << setw(5) << dime << " (" << "$0." << dimevalue << ")" << "\n";
                }
 
 
                if (nickelvalue == 0)
                {
                        // If nickelvalue is = to 0, don't display.
                }
 
                // If we only have a single nickle, we fix the decimal point output.
                else if (nickelvalue == 5)
                {
                        cout << " Nickels:" << setw(3) << nickel << " (" << "$0.0" << nickelvalue << ")" << "\n";
 
                }
 
                else
                {
                        cout << " Nickels:" << setw(3) << nickel << " (" << "$0." << nickelvalue << ")" << "\n";
                }
 
 
                if (pennyvalue == 0)
                {
                        // If pennyvalue is = to 0, don't display.
                }
 
                else
                {
                        cout << setw(2) << " Pennies:" << setw(3) << penny << " (" << "$0.0" << pennyvalue << ")" << "\n";
                }
 
                }
 
                // If change is = to 0, don't run the entire program, just run this portion.
                else
                {
                        cout << "Change Due" << "$0.00" << "\n";
                        cout << "Coin Dispenser will dispense:\n";
                        cout << "No Coins\n";
                }
 
}
Last edited on
MiiNiPaa (8886)
1) This is not a random number generation.
Here it is:
1
2
 
srand(time(NULL));
unsigned change = rand() % 100;


2) Use fixed and setprecision(2) manipulators to avoid 2 cases in your output (lines 30-40)

3) You do not actually need conditions for checking if sum is large enough for quarters/dimes/etc. Everything will work correctly even without it

4) You cannot have 2 nickels, as they would be replaced by dime (lines 108-111)

5) It is generally a bad style to have then block empty and writing code in else. (lines 74, 85, 96, 114). Use not equal (!=) to compare instead.

6) use better identation. Lines 67 and 127 does not looks like they are related.

7) I would make xxxvalue variables constant and made them contain value of single coin. Actual value would be xxx * xxxvalue
mutexe (2372)
Last edited on
soranz (536)
Hi there ! Looks good. Lots of commenting. Everything flows consistently. I didn't test it but it looks like it does what it's supposed to do.

If you're looking for some ideas:
line 133: should have return 0; because the function is int main()

This will probably be in the next section at school but you'll notice that there's a lot of similar things going on like: 
1
2
3
 
quarters = change / 25;
change = change % 25;
quartervalue = quarters * 25;

All groups like that could be put into a simple function.
As well as many of the cout calls.

Just be aware that truncating a type like int with divisions can potentially cause unexpected rounding:
nickel = change / 5;
Last edited on
MiiNiPaa (8886)
line 133: should have return 0; because the function is int main()
It is not should, it is may. Standard explicitely allows to not have return statement in main()

Just be aware that truncating a type like int with divisions can potentially cause unexpected rounding
This is actually what is needed there. And it is not unexpected: integer division is well defined and documented.

Your code with everyhing changed according to my suggestions (and more): http://ideone.com/wm9vUo

There is more to add, like adding data structure to hold coin description, and making everything automatic without need to copypatse anything.

EDIT: Just for fun — avoiding copypaste http://ideone.com/ELhL6g (still not perfect. There is much to change)
Last edited on
mishappp (59)
haha, some of these things are a little advanced or a tad confusing to me. The teacher told us that you are not suppose to use namespace and went over namespaces but said that we should use it just for the sake of being able to read and understand the code. He also said we should not go on the internet and look up methods of doing things that we haven't covered. So I took some / most of the advice I could understand and I tried again. Also the "random number generator" was given to us through a file, it worked for me. But why is yours better?

http://ideone.com/LXPcDf


Thanks again for all the tips.
Last edited on
MiiNiPaa (8886)
Lines related to "generating" a number;
1
2
3
 
unsigned change = time(0);
srand(change);
change = change % 100;


srand(change); sets a seed for use in rand() function but you do not call it anywhere, so this line essentually does nothing

1
2
 
unsigned change = time(0);
change = change % 100;
can be simplified to unsigned change = time(0) % 100;
Bascally you are generating value based on last two digits of time() result. So you can manipulate generated number by executing program in right time. You have only one number generated and it is not noticeable, but if you would want to generate more, you will have problems

My approach is not that good either. You should use C++ faculties from <random> and/or discard several first values (because of problem with some implementations of rand()). I just quickly fixed glaing error.
Last edited on
Topic archived. No new replies allowed.