C++

Forum

 
Program Evaluation

DeadH34d (69)
Good Afternoon everybody. I just made a Chore List program, and I would like to know if anything I did was bad practice or cluttered, etc. Basically, I'm looking for a critique. 
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
135
136
137
138
 
#include <iostream>
#include <stdlib.h>
#include <string>
using namespace std;

int main(int argc, char *argv[])
{
//The following strings are the chores that the user needs done.
  string chore1;
  string chore2;
  string chore3;
  string chore4;
  string chore5;
  int complete; //This integer is used to show which chores have been finished.
  
  cout << "List any 5 chores that need to be done today. \n";
  cout << "After all five chores have been entered,\n";
  cout << "type in their respective number, and the \n";
  cout << "chore will be added to the finished list.\n";
  cout << "\n";
  
  cout << "===CHORE===LIST===\n";
  
  cout<< "\n";
  //This is where the chores are entered and listed.
  cout << "1) ";
  getline (cin,chore1);
  cout << "2) ";
  getline (cin,chore2);
  cout << "3) ";
  getline (cin,chore3);
  cout << "4) ";
  getline (cin,chore4);
  cout << "5) ";
  getline (cin,chore5);
  
  cout << "\n";
  
  cout << "=====FINISHED=====\n";
  
  cin >> complete; //This is where you enter which chore has been finished.
  
  if (complete == 1) {
    cout << "1) " << chore1 << "\n";
    }
  else if (complete == 2) {
    cout << "1) " << chore2 << "\n";
    }
  else if (complete == 3) {
    cout << "1) " << chore3 << "\n";
    }
  else if (complete == 4) {
    cout << "1) " << chore4 << "\n";
    }
  else if (complete == 5) {
    cout << "1) " << chore5 << "\n";
    }
  
  cin >> complete;
  
  if (complete == 1) {
    cout << "2) " << chore1 << "\n";
    }
  else if (complete == 2) {
    cout << "2) " << chore2 << "\n";
    }
  else if (complete == 3) {
    cout << "2) " << chore3 << "\n"; 
    }
  else if (complete == 4) {
    cout << "2) " << chore4 << "\n";
    }
  else if (complete == 5) {
    cout << "2) " << chore5 << "\n";
    }
  
  cin >> complete;
  
  if (complete == 1) {
    cout << "3) " << chore1 << "\n"; 
    }
  else if (complete == 2) {
    cout << "3) " << chore2 << "\n";
    }
  else if (complete == 3) {
    cout << "3) " << chore3 << "\n";
    }
  else if (complete == 4) {
    cout << "3) " << chore4 << "\n";
    }
  else if (complete == 5) { 
    cout << "3) " << chore5 << "\n"; 
    }
    
  cin >> complete;
  
  if (complete == 1) {
    cout << "4) " << chore1 << "\n";
    }
  else if (complete == 2) {
    cout << "4) " << chore2 << "\n";
    }
  else if (complete == 3) {
    cout << "4) " << chore3 << "\n";
    }
  else if (complete == 4) {
    cout << "4) " << chore4 << "\n";
    }
  else if (complete == 5) {
    cout << "4) " << chore5 << "\n";
    }
    
  cin >> complete;
  
  if (complete == 1) {
    cout << "5) " << chore1 << "\n";
    }
  else if (complete == 2) {
    cout << "5) " << chore2 << "\n";
    }
  else if (complete == 3) {
    cout << "5) " << chore3 << "\n";
    }
  else if (complete == 4) {
    cout << "5) " << chore4 << "\n";
    }
  else if (complete == 5) {
    cout << "5) " << chore5 << "\n";
    }
  
  cout << "\n";  
  
  cout << "All chores are finished.\n"; //Finally, the program informs you when all chores are finished.
  
          
  system("PAUSE");	
  return 0;
}


Just off the top of my head, I think it would've been more organized and quicker with switch statements. If statements seemed easier at the time. *shrugs*
Disch (13742)
you should use arrays instead of renaming the same variable to have different numbers.

IE:

1
2
3
4
5
6
7
8
 
// This is bad
string chore1;
string chore2;
 //...  etc


// This is better
string chores[5];


The benefit to this is now you have all 5 strings accessable by the same name. To get individual strings, you just give an index. This lets you put the code in loops to compact it.

1
2
3
4
5
6
7
8
9
10
11
12
13
 
//This is bad
cout << "1) ";
getline (cin,chore1);
cout << "2) ";
getline (cin,chore2);
  // ... etc

// This is better
for(int i = 0; i < 5; ++i)
{
  cout << i+1 << ") ";
  getline(cin,chores[i]);
}


It reduces the amount of code, and prevents copy/paste errors and duplicate code.

What's more, you can then get rid of this crap as well:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
// This is bad
  if (complete == 1) {
    cout << "1) " << chore1 << "\n";
    }
  else if (complete == 2) {
    cout << "1) " << chore2 << "\n";
    }
   // ... etc

// This is better
  if(complete >= 1 && complete <= 5)
  {
    cout << "1) " << chores[complete-1] << "\n";
  }


Lastly, instead of copy/pasting the same cin >> complete and following code 5 times, just write it one time and put it in a loop that runs 5 times.



EDIT:

Here's the exact same program, only written to use loops and arrays instead of individual variables and copy/pasted code:

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
 
#include <iostream>
#include <cstdlib>
#include <string>
using namespace std;

int main(int argc, char *argv[])
{
//The following strings are the chores that the user needs done.
  string chores[5];
  int complete; //This integer is used to show which chores have been finished.

  cout << "List any 5 chores that need to be done today. \n";
  cout << "After all five chores have been entered,\n";
  cout << "type in their respective number, and the \n";
  cout << "chore will be added to the finished list.\n";
  cout << "\n";

  cout << "===CHORE===LIST===\n";

  cout<< "\n";
  
  //This is where the chores are entered and listed.
  for(int i = 0; i < 5; ++i)
  {
      cout << i+1 << ") ";
      getline(cin,chores[i]);
  }

  cout << "\n";

  cout << "=====FINISHED=====\n";

  for(int i = 1; i <= 5; ++i)
  {
      cin >> complete; //This is where you enter which chore has been finished.
      if(complete >= 1 && complete <= 5)
        cout << i << ") " << chores[complete-1] << "\n";
  }

  cout << "\n";

  cout << "All chores are finished.\n"; //Finally, the program informs you when all chores are finished.


  return 0;
}


You'll notice the code is easier to read and is much smaller.
(I also got rid of the system("pause") out of principle)
Last edited on
DeadH34d (69)
Yeah, it's about 100 lines smaller. haha. I never woulda thought to do it like that. Actually now that I look at it... I'm kinda confused. You'll have to forgive me; I'm quite new to this.
I don't understand this part particularly:
1
2
3
4
 
{
  cout << i+1 << ") ";
  getline(cin,chores[i]);
}

why chores[i]? I understand what the for loop does, but I don't get why the chores index is i.

and this part is a little strange to me:
1
2
3
 
{
    cout << "1) " << chores[complete-1] << "\n";
  }

again, I understand what the loop does and how it works for the most part, but the chore[complete-1] throws me off.

Thanks for the help, I hope I'm not asking too many questions.
Disch (13742)
why chores[i]?


chores is an array of strings. Each "element" in the array is an individual string. Those elements are accessed via an "index". The index is basically the ID number of the string you want to access.

For example, you could say that:

1
2
3
4
 
chore1 is the same as chores[0]
chore2 is the same as chores[1]
chore3 is the same as chores[2]
... etc


The thing to note here is that the index is zero based. IE: chores[0] is the first element in the array, not chores[1]

chores[i] uses the loop counter 'i' as the index. Since the loop counter changes each time through the loop, it means we're accessing a different string each time the loop runs. So....

1
2
3
4
5
 
  for(int i = 0; i < 5; ++i)
  {
      cout << i+1 << ") ";
      getline(cin,chores[i]);  // <- which string are we accessing here?
  }

The first time that loop runs, i == 0, therefore we are getting chores[0]
but the next time it runs, i == 1, so we're getting chores[1] instead
ie: each time the loop runs, we're getting a different string. A different element in the array.


again, I understand what the loop does and how it works for the most part, but the chore[complete-1] throws me off.


It's the same idea. Instead of having an if/else chain checking to see which number the user entered, we just use that number directly to access the appropriate string.

IE if the use entered "4", then complete == 4, and therefore we print chores[3] (ie: the 4th string in the array)

The -1 is there because the array is zero based, but 'complete' is 1 based. The -1 basically "translates" 1-based to 0-based

Thanks for the help, I hope I'm not asking too many questions.


You're not. It's nice to speak with people who actually want to learn, rather than people looking for the fastest way to solve their homework problem.

Feel free to ask any other questions you may have. I'm happy to clarify things that are still confusing to you.
DeadH34d (69)
Ohhh, ok. That makes perfect sense. thank you. Trust me, all the help is greatly appreciated.

Edit: Ah, it's been a good day. Learned some things with this chore list program, and then I sat down and did this and it worked great first time. 
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
 
#include <iostream>
#include <stdlib.h>
using namespace std;

int main(int argc, char *argv[])
{
  float balance = 173.96;
  int choice;
  float money;
  
    cout << "===A=T=M===\n";
    cout << "\n";
    cout << "Balance: $" << balance << "\n";
    cout << "Please select an option.\n";
    cout << "1) Deposit\n";
    cout << "2) Withdraw\n";
    cin >> choice;
    switch (choice) {
        case 1:
                cout << "How much would you like to deposit?\n";
                cin >> money;
                balance += money;
                cout << "Balance: $" << balance << "\n";
                break;
        case 2:
                cout << "How much would you like to withdraw?\n";
                cin >> money;
                balance -= money;
                if (balance < 0) {
                    cout << "Insufficient funds\n";
                    break;
                }
                cout << "Balance: $" << balance << "\n";
                break;
        default:
                cout << "Please make a valid choice.\n";
                break;
        }
  system("PAUSE");	
  return 0;
}
Last edited on
Topic archived. No new replies allowed.