C++

Forum

 
Code Review

Ineff (3)
I am in the process of learning C++, the code i have made is a basic calculator and a number guessing game. I am trying to get a full grip on classes and objects at the moment. Anyone willing to critic the code and give any pointers or better practices.

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
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
 
#include "pch.h"
#include <iostream>
#include <string>
#include "DoCalc.h"
#include "GuessNumbers.h"

using namespace std;
		
int a;

int main()
{
	cout << "Press 1 to perform Calculations or Press 2 for Guess the Number" << endl;
	cin >> a;
	if (a == 1) {
		DoCalc DoingCalc;
		DoingCalc.DoCalculation();
	}
	else if (a == 2) {
		GuessNumbers GuessNum;
		GuessNum.GuessingNumbers();
	}

}
 
DoCalc Class
---------------
#pragma once
#include <string>
#include <iostream>

using namespace std;

class DoCalc {
public:
	string input;
	bool valid = true;
	bool again = true;

	int add() 
	{
		int x, y, sum;
		cout << "Enter first number to add" << endl;
		cin >> x;
		cout << endl;
		cout << "Enter second number to add" << endl;
		cin >> y;
		sum = x + y;
		cout << "Your answer is " << sum << endl;
		valid = false;
		return sum;
	}

	int subtract()
	{
		int x, y, sum;
		cout << "Enter first number to subtract" << endl;
		cin >> x;
		cout << endl;
		cout << "Enter second number to subtract" << endl;
		cin >> y;
		sum = x - y;
		cout << "Your answer is " << sum << endl;
		valid = false;
		return sum;
	}

	int multiply()
	{
		int x, y, sum;
		cout << "Enter first number to multiply" << endl;
		cin >> x;
		cout << endl;
		cout << "Enter second number to multiply" << endl;
		cin >> y;
		sum = x * y;
		cout << "Your answer is " << sum << endl;
		valid = false;
		return sum;
	}

	int divide()
	{
		int x, y, sum;
		cout << "Enter first number to divide" << endl;
		cin >> x;
		cout << endl;
		cout << "Enter second number to divide" << endl;
		cin >> y;
		sum = x / y;
		cout << "Your answer is " << sum << endl;
		valid = false;
		return sum;
	}


	void DoCalculation()
	{
		do {
			cout << "Add, Subtract, Multiply, Divide" << endl;
			cin >> input;
			for (string::iterator i = input.begin(); i < input.end(); i++) {
				*i = tolower(*i);
			}
			if (input == "add") {
				add();
			}
			else if (input == "subtract") {
				subtract();
			}
			else if (input == "multiply") {
				multiply();
			}
			else if (input == "divide") {
				divide();
			}
			else {
				cout << "Please check spelling and try again" << endl;
			}

			do {
				cout << "Would you like to perform another calculation?" << endl;
				cout << "Yes or No" << endl;
				cin >> input;
				for (string::iterator i = input.begin(); i < input.end(); i++) {
					*i = tolower(*i);
				}
				if (input == "yes") {
					DoCalc();
				}
				else if (input == "no") {
					valid = false;
					again = false;
				}
				else {
					cout << "Please enter either Yes or No" << endl;
				}
			} while (again == true);

		} while (valid == true);
	}

};

-----------
GuessNumbers class
-----------
#pragma once
#include <iostream>
#include <random>
#include <time.h>
#include <string>

using namespace std;

class GuessNumbers
{
public:
	int a, random, numOfGuesses = 0;
	string input;

	int randomgen(int max, int min)
	{
		srand(time(NULL));
		random = rand() % max + min;
		return random;
	}

	void GuessingNumbers()
	{
		randomgen(100, 1);
		cout << "Im thinking of a number between 1 & 100" << endl;
		cout << "Can you guess it?" << endl;
		do {
			cin >> a;
			if (a > random) {
				cout << "Guess is to high, Guess again!" << endl;
				numOfGuesses++;
			}
			else if (a < random) {
				cout << " Guess is to low, Guess again!" << endl;
				numOfGuesses++;
			}
			else if (a == random) {
				numOfGuesses++;
				cout << "Congrats you Guessed Correctly!" << endl;
				cout << "It took " << numOfGuesses << " to guess correctly" << endl;
			}
		} while (a != random);
	}

};
Handy Andy (5051)
Hello Ineff,

Thank you for using code tags, but it could have been done a little better. Each header should have had its own set of code tags, but it works this way too.


Since "main" is first I start there.

1
2
3
4
5
6
7
8
9
 
#include "pch.h" // <--- Not necessary when posting code here.
#include <iostream>
#include <string>

#include "DoCalc.h"
#include "GuessNumbers.h"

//using namespace std;  // <--- Best not to use.
// A recent post that is worth reading. http://www.cplusplus.com/forum/beginner/258335/

Line 1 does not help anyone here and it often gets comments that it is not a standard C++ header file. If you learn to create an empty project you will not have that line in the file because you will have to create the "main" file and add the header files that you need.

Lines 8 and 9 should explain them selves.

Time and experience have taught me that this line produces a better looking prompt.
1
2
 
std::cout << "\n Press\n 1 to perform Calculations\n 2 for Guess the Number\n Enter choice: ";
std::cin >> a;


Avoid using global variables unless they start with "constexpr" or "const". As a global variable with file scope anything, meaning any function, in the file has access to this variable and can change it making it herder to figure out where it went wrong.

With main being short as it is "a" should be defined inside "main" where it is used.

And when it comes to variable names try to avoid single letter names. As your programs grow it is harder to keep track of what they are and being used for. A name like "choice" or "input" is more descriptive and easier to understand. Quite often I use "choice" for something like this.

Also some well placed blank lines help in readability.

I do not know if you are at a point of catching errors yet, but this code is one possibility of what you can do:
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
 
#include <iostream>
#include <limits>  // <--- Added.
#include <string>

#include "DoCalc.h"
#include "GuessNumbers.h"

//using namespace std;  // <--- Best not to use.
// A recent post that is worth reading. http://www.cplusplus.com/forum/beginner/258335/

int main()
{
	int choice;
	DoCalc DoingCalc;       // <--- Moved to here.
	GuessNumbers GuessNum;  // <--- Moved to here.

	std::cout << "\n Press\n 1 to perform Calculations\n 2 for Guess the Number\n 3 Exit\n Enter choice: ";
	std::cin >> choice;
	
	while (!std::cin || (choice < 1 || choice > 3))
	{
		if (!std::cin)
		{
			std::cout << "\n    Invalid entry! Needs to be a number.\n";
			std::cin.clear();
			std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');  // <--- Requires header file <limits>.
		}
		else if (choice < 1 || choice > 3)
		{
			std::cout << "\n    Invalid entry! Choices are 1, 2 or 3\n";
		}

		std::cout << "\n Press\n 1 to perform Calculations\n 2 for Guess the Number\n 3 Exit\n Enter choice: ";
		cin >> choice;
	}

	if (choice == 1)
	{
		//DoCalc DoingCalc;
		DoingCalc.DoCalculation();
	}
	else if (choice == 2)
	{
		//GuessNumbers GuessNum;
		GuessNum.GuessingNumbers();
	}

	//switch (choice)
	//{
	//	case 1:
	//		DoingCalc.DoCalculation();
	//		break;
	//	case 2:
	//		GuessNum.GuessingNumbers();
	//		break;
	//	case 3:
	//		break;
	//	default:
	//		break;
	//}

	return 0;
}

This is just a suggestion. Your code will work as best I can tell before testing.

I added the "switch" as an alternative. The "switch" would be a better choice unless you can not use it or are not ready for it yet.

The "return" is optional and not necessary, but makes a good break point when debugging. I do not know how you have set up your VS, but in mine when the "return" is executed the window the program is running in closes. Making it hard to see any last messages displayed.

I use this quite often at the end of "main":
1
2
3
4
 
// The next line may not be needid. If you have to press enter to see the prompt it is not needed.
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');  // <--- Requires header file <limits>.
std::cout << "\n\n Press Enter to continue: ";
std::cin.get();

I have found it useful.

That should cover "main" and give you something to think abour while I look over the two header files.

Hope that helps,

Andy
jonnin (11333)
time.h is C use <ctime>

you should separate your input and output from your work.
I know this is a small program but this is huge. Lets say next week you want to put a GUI on this instead of having a cin/cout type console program. Oh no, you have to rewrite add, because add isn't just adding, it is also doing I/O! So are the others! Look at how many places you have to make changes. The I/O should be in a function.

consider:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
class DoCalc {
int x,y; //private!
public:
	string input;
	bool valid = true;
	bool again = true;      
        
	int add() 
	{   //sum;
		valid = false; //why? I didn't look into what this is for but left it in
		return x + y;
	}
       
 void getxy(string s) //input
 {      
cout << "Enter first number to " << s <<  endl; //s is word "add" or whatever. 
		cin >> x;
		cout << endl;
		cout << "Enter second number to " << s << endl;
		cin >> y;		
}
//output
void showresult(int result) {cout << "Your answer is " << result << endl;}

and you would call those down in your docalculation logic.
then, if you changed the code to use a GUI, you just need to rewrite 2 functions, not nearly the whole program, which would be a massive thing for a large scale program, though it would be not much trouble for this small program, hopefully you see this?

you don't need add, subtract, etc at all, by the way. you can now remove them after doing the above, and just say int result = x+y down in docalculation and manage valid there as well. Its not worth the code bloat to have these functions now, in this instance, though these can serve as examples where a more complicated block of code would go.

switches, mentioned above, are worth a little discussion too.
switches work only off integers, which is very, very limiting. They require a 'break' between statements if you want them distinct, or you can leave it off to 'fall through'. that is
switch (x)
case : a
case : b //this executes if it is a because a did not have a break.
case: c break;
case: d whatever; //d does not execute if the case is c because of break.

switches can be significantly more efficient than if/then chains, if you have an integer or can easily make one that governs the logic. They are internally compiled into more efficient code in many instances.

Last edited on
Handy Andy (5051)
Hello Ineff,

Well jonnin seems to have gotten farther than I have and makes very good points.

So far I have worked on the "DoCalc.h" file and this is what I see.

Try to avoid using the #pragma once . I believe not everyone can use it. A better choice is header guards that I will show later.

The header files:
1
2
 
#include <string>
#include <iostream>

In this program these should be in "main".

using namespace std; should never be put or needed in a header file. Read this http://www.lonecpluspluscoder.com/2012/09/22/i-dont-want-to-see-another-using-namespace-xxx-in-a-header-file-ever-again/

A note on using {}s. Whichever method you choose be consistent in the use. It really breaks up the rhythm when reading and trying to keep track of the {}s. Of the choices I prefer the "Allman" style https://en.wikipedia.org/wiki/Indentation_style It seems to be the easiest to read and work with, at least for me. In the end it it your choice and what you are comfortable with.

When it comes to classes it is more common to put the class in a header file and the functions in a ".cpp" file. What you have now works, but keep this in mind for the future.

In the class putting everything under "public:" works, but in a sense it kind of defeats the purpose of keeping the variables protected form code anywhere in the program from changing them.

When I gave the program its first run it showed me this:

 Press
 1 to perform Calculations
 2 for Guess the Number
 3 Exit
 Enter choice: 1
Add, Subtract, Multiply, Divide

First it gives you an idea of how changing the first prompt looks. But when the last line came up I was not sure what needed to be entered.

That line Add, Subtract, Multiply, Divide is nice, but you are expecting the user to know how to spell correctly and counting on it. And what happens if the user puts "sub" for subtract. It will not work. Now trying to think of every possibility of what to check for you are bound to miss something and that is what someone will use to break the program. A numbered menu keeps it simple and easy to use. Best to keep it simple if you can.

Again a numbered menu would work better. This std::cout << "\n 1. Add\n 2. Subtract\n 3. Multiply\n 4. Divide\n Enter choice: "; gave the following output on the screen:

 Press
 1 to perform Calculations
 2 for Guess the Number
 3 Exit
 Enter choice: 1

 1. Add
 2. Subtract
 3. Multiply
 4. Divide
 Enter choice:

To this I would add a fifth choice to exit. Notice how the end of the string is choice: " . The space after the ":" alone with dropping the "std::endl", which puts the "std::cin" at the end of the line and not the next line, makes for a better prompt IMHO.

Also that would mean changing std::string input; to size_t choice;. "size_t" AKA "unsigned int" because you should be working positive numbers only, but an "int" will be just fine and should you allow negative numbers the "int" is a better choice.

In the all four functions int x, y, sum;, but "x" and "y" tend to make me thing of points on a graph. A better possible choice could be int num1{}, num2{}, result{}; The {}s, available form C++11 on, initialize the variables to (0) zero. Not always necessary, but does give peace of mind that they do not contain a garbage value. If you are not doing addition "sum" is a bit misleading.

In your division function you have defined your variables as "int"s which means that you are doing integer division. This may not always produce the results that you need. Example: 1 / 4 = 0.025, but storing that in an int would give you (0) zero because it would store the whole number and drop the decimal part. Then you are asking why it prints (0) zero.

Another thing you should do is check that "num2" is greater then (0) zero otherwise you will get the run time error "divide by 0" and your program would stop.
This is what"DoCal.h" could look like:
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
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
 
#ifndef DOCALC_H
#define DOCALC_H

class DoCalc 
{
		std::string input;
		bool valid = true;
		bool again = true;

	public:
		int add()
		{
			int x, y, sum;

			std::cout << "\n Enter first number to add" << std::endl;
			std::cin >> x;
			//std::cout << std::endl; // <--- Start the next line with "\n".

			std::cout << "\n Enter second number to add" << std::endl;
			std::cin >> y;

			sum = x + y;

			std::cout << "\n Your answer is " << sum << std::endl;

			valid = false;

			return sum;
		}

		int subtract()
		{
			int x, y, sum;

			std::cout << "Enter first number to subtract" << std::endl;
			std::cin >> x;
			std::cout << std::endl;

			std::cout << "Enter second number to subtract" << std::endl;
			std::cin >> y;

			sum = x - y;

			std::cout << "Your answer is " << sum << std::endl;

			valid = false;

			return sum;
		}

		int multiply()
		{
			int x, y, sum;

			std::cout << "Enter first number to multiply" << std::endl;
			std::cin >> x;
			std::cout << std::endl;

			std::cout << "Enter second number to multiply" << std::endl;
			std::cin >> y;

			sum = x * y;

			std::cout << "Your answer is " << sum << std::endl;

			valid = false;

			return sum;
		}

		int divide()
		{
			int x, y, sum; // <--- Would work better as doubles.

			std::cout << "Enter first number to divide" << std::endl;
			std::cin >> x;
			std::cout << std::endl;

			std::cout << "Enter second number to divide" << std::endl;
			std::cin >> y;

			// <--- Check for greater than (0) zero or "!=" (0) zero.

			sum = x / y;

			std::cout << "Your answer is " << sum << std::endl;

			valid = false;

			return sum;
		}


		void DoCalculation()
		{
			do
			{
				//std::cout << "Add, Subtract, Multiply, Divide" << std::endl;
				std::cout << "\n 1. Add\n 2. Subtract\n 3. Multiply\n 4. Divide\n Enter choice: ";
				std::cin >> input;

				for (std::string::iterator i = input.begin(); i < input.end(); i++)
				{
					*i = std::tolower(*i);
				}

				// <--- An alternative, but neither should be used.
				//for (size_t i = 0; i < input.size(); i++)
				//{
				//	std::tolower(input[i]);
				//}

				if (input == "add")
				{
					add();
				}
				else if (input == "subtract")
				{
					subtract();
				}
				else if (input == "multiply")
				{
					multiply();
				}
				else if (input == "divide")
				{
					divide();
				}
				else
				{
					std::cout << "Please check spelling and try again" << std::endl;
				}

				do
				{
					std::cout << "Would you like to perform another calculation?" << std::endl;
					std::cout << "Yes or No" << std::endl;
					std::cin >> input;

					for (std::string::iterator i = input.begin(); i < input.end(); i++)
					{
						*i = std::tolower(*i);
					}
					if (input == "yes")
					{
						DoCalc();
					}
					else if (input == "no")
					{
						valid = false;
						again = false;
					}
					else
					{
						std::cout << "Please enter either Yes or No" << std::endl;
					}
				} while (again == true);  // End 2nd do/while.

			} while (valid == true);  // End 1st do/while
		}  // End function DoCalculation()
};

#endif // !DOCALC_H

Lines 1,2 and 163 are the header guard I mentioned earlier.

"public:" is moved down to line 10 making the variables "private" because a class is "private" by default. The rest of the code has comments you should read.

The for loop to change case, which you really do not need, shows a simpler way of doing what you did. You do not need to be as fancy and I think what you have shoold work.

I know that this may not be what you are expecting, but as a whole it all goes together.

Almost forgot. Notice the blank lines to break up the code. it makes it much easier to read. The compiler does not care about white space, blank lines or comments, but someone reading, including your-self, it does make a difference.

Hope that helps,

Andy
Ineff (3)
Thanks for the replies, and sorry about the long delay. Lots of driving in the next week for me.

I have refactored main and calculator, i think i caught everything both of you pointed out.

The header files:
1
2
 
1 #include <string>
2 #include <iostream>


In this program these should be in "main".

I get 103 errors if i don't use #include <iostream> in my header. So i don't know what to do to not include it.

This is now main and calculator.h and .cpp

Main
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
 
#include <iostream>
#include <string>
#include <limits>
#include "Calculator.h"
#include "Calculator.h"


int main()
{
	int selection;
	Calculator DoingCalc;
	bool again = true;
	//GuessNumGame GuessNum; // Refactoring
	do
	{
		std::cout << "\n Press \n 1 Calculator \n 2 Guess the Number ------------- Currently Inoperable \n 3 Exit \n Choice:";
		std::cin >> selection;

		while (!std::cin || (selection < 1) || (selection > 3))
		{
			if (!std::cin)
			{
				std::cout << "\n Invalid entry! Please enter a number.\n";
				std::cin.clear();
				std::cin.ignore(std::numeric_limits < std::streamsize>::max(), '\n');
			}
			else if (selection < 1 || selection > 3)
			{
				std::cout << "\n   Invalid entry! Choices are 1, 2, or 3\n";
			}

			std::cout << "\n Press \n 1 Calculator \n 2 for Guess the Number \n 3 Exit \n Enter choice: ";
			std::cin >> selection;
		}

		switch (selection)
		{
		case 1:
			DoingCalc.selectOperation();
			break;
		case 2:
			//GuessNum.GuessingNumbers(); //Refactoring
			break;
		case 3:
			again = false;
			break;
		default:
			break;
		}

	} while (again);

		std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
		std::cout << "\n\n Press Enter to continue: ";
		std::cin.get();

}


Calculator.h
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
#ifndef Calculator_H
#define Calculator_H
#include <iostream>

class Calculator
{
//private:
	std::string operation;
	int selection;
	double num1, num2, result;

public:
	void selectOperation();

	void getNums();

	void doCalculation();

	void showResults(double results);

};

#endif


Calculator.cpp
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
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
 
#include "Calculator.h"


void Calculator::selectOperation()
{
	std::cout << "\n Press \n 1 Add \n 2 Subtract \n 3 Multiply \n 4 Divide \n Choice:";
	std::cin >> selection;
	std::cout << std::endl;

		while (!std::cin || (selection < 1) || (selection > 4))
		{
			if (!std::cin)
			{
				std::cout << "\n Invalid entry! Please enter a number.\n";
				std::cin.clear();
				std::cin.ignore(std::numeric_limits < std::streamsize>::max(), '\n');
			}
			else if (selection < 1 || selection > 4)
			{
				std::cout << "\n   Invalid entry! Choices are 1, 2, 3, or 4\n";
			}

			std::cout << "\n Press \n 1 Add \n 2 Subtract \n 3 Multiply \n 4 Divide \n Choice:";
			std::cin >> selection;
		}

		switch (selection)
		{
			case 1:
				operation = "add";
				getNums();
				break;

			case 2:
				operation = "subtract";
				getNums();
				break;

			case 3:
				operation = "multiply";
				getNums();
				break;

			case 4:
				operation = "divide";
				getNums();
				break;

			default:
				break;
		}
}

void Calculator::getNums()
{
	std::cout << "Enter the first number to " << operation << std::endl;
	std::cin >> num1;
	while (!std::cin)
	{
		if (!std::cin)
		{
			std::cout << "\n Invalid entry! Please enter a number.\n";
			std::cin.clear();
			std::cin.ignore(std::numeric_limits < std::streamsize>::max(), '\n');
		}
		std::cout << "Enter the first number to " << operation << std::endl;
		std::cin >> num1;
	}
	std::cout << "Enter the second number to " << operation << std::endl;
	std::cin >> num2;
	while (!std::cin)
	{
		if (!std::cin)
		{
			std::cout << "\n Invalid entry! Please enter a number.\n";
			std::cin.clear();
			std::cin.ignore(std::numeric_limits < std::streamsize>::max(), '\n');
		}
		std::cout << "Enter the second number to " << operation << std::endl;
		std::cin >> num2;
	}
	doCalculation();
}

void Calculator::doCalculation()
{
	switch (selection)
	{
	case 1:
		result = num1 + num2;
		showResults(result);
		break;

	case 2:
		result = num1 - num2;
		showResults(result);
		break;

	case 3:
		result = num1 * num2;
		showResults(result);
		break;

	case 4:
		if (num1 ==0 || num2 == 0)
		{
			result = 0;
		}
		else
		{
			result = num1 / num2;
		}
		showResults(result);
		break;

	default:
		break;
	}
}

void Calculator::showResults(double result)
{
	std::cout << "\n Your answer is " << result;
	std::cout << std::endl;
	std::cout << "\n Would you like to perform another action?" << std::endl;
	std::cout << "\n Press \n 1 Yes \n 2 No \n Choice : ";
	std::cin >> selection;

	while (!std::cin || (selection < 1) || (selection > 2))
	{
		if (!std::cin)
		{
			std::cout << "\n Invalid entry! Please enter a number.\n";
			std::cin.clear();
			std::cin.ignore(std::numeric_limits < std::streamsize>::max(), '\n');
		}
		else if (selection < 1 || selection > 2)
		{
			std::cout << "\n   Invalid entry! Choices are 1, 2, 3, or 4\n";
		}

		std::cout << "\n Press \n 1 Add \n 2 Subtract \n 3 Multiply \n 4 Divide \n Choice:";
		std::cin >> selection;
	}

	switch (selection)
	{
	case 1:
		selectOperation();
		break;

	case 2:
		break;

	default:
		break;
	}
}


Thanks Ineff
Last edited on
MikeyBoy (5631)
I get 103 errors if i don't use #include <iostream> in my header. So i don't know what to do to not include it.

You definitely need to include <string> in calculator.h, because you use the class in that header.

There's nothing in calculator.h the requires <iostream>. Obviously, both your main.cpp and calculator.cpp files do need it, so it should be included in both those files.
Last edited on
jonnin (11333)
includes can be tricky... if A includes B that includes C, A can see C and if A requires C, and you remove B, A stops working. That is likely what happened. You just bridge the gap by putting C into A directly instead of inheriting it.
Ineff (3)
So
1
2
3
 
#ifndef Calculator_H
#define Calculator_H
#include <string> 


1
2
 
#include "Calculator.h"
#include <iostream>

works fine. Is this what you are referring to?
MikeyBoy (5631)
If the first section is from calculator.h, and the second section is from calculator.cpp, then yes. I hope it's obvious why this is right?
dhayden (5795)
Create a function to prompt for a number within some limits.

Currently selectOperation() calls getNums() which calls doCalculation() which calls showResults() which calls SelectOperation(). Reading the method names, I'd never guess that calling selectOperation would do everything. Change these functions so that each one does just what it says. To chain their functionality together, call each one in turn.

Also add a brief comment to say what each function does. This helps to keep things clear in your head and also discourages hacks that "just get it to work."

When I added comments, I quickly discovered that selectOperation() shoudl be called getSelection() instead.

Use simple nouns and verbs for names. I'd change these:
1
2
 
Calculator DoingCalc;
Calculator::doCalculation()

to
1
2
 
Calculator calc;
Calculator::calculate()


The calculator should just calculate. Whether the user wants to perform another action is the business of the main program, not the calculator.

I put this all in one file for my own convenience.
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
139
140
141
142
143
 
#ifndef Calculator_H
#define Calculator_H
#include <iostream>
#include <limits>

class Calculator
{
//private:
    std::string operation;    // "add" "subtract" "multiply" or "divide"
    int selection;		// 1=add 2=subtract 3=multiply, 4=divide
    double num1, num2;		// the operands. We'll do num1 <operator> num2
    double result;		// the result of num1 <operator> num2

  public:
    void getSelection();	// get the selection from the user
    void getNums();		// get the two numbers from the user
    void calculate();		// calculate result
    void showResults();		// show result
};

#endif

// Prompt for an integer between min and max (min <= num < max) Keep
// prompting until the user gives a valid response. Return the valid
// response.
int getNum(const char *prompt, int min, int max)
{
    int result;

    // This is a case where the condition of the loop is actually midway
    // through the loop code.  Don't be afraid to do this when the logic
    // calls for it
    while (true) {
	std::cout << prompt;
	std::cin >> result;

	// Here is the loop condition
	if (std::cin && min <= result && result < max) {
	    return result;
	}

	// Invalid input
	std::cout << "\n Invalid entry! Please enter a number from "
		  << min << " to " << max-1 << '\n';
	std::cin.clear();
	std::cin.ignore(std::numeric_limits < std::streamsize >::max(), '\n');
    }
}


void Calculator::getSelection()
{
    // with getNum, this becomes a one-liner
    selection = getNum("\n Press \n 1 Add \n 2 Subtract \n 3 Multiply \n 4 Divide \n Choice:", 1, 5);
}

void
Calculator::getNums()
{
    // Set the "operation" string based on the selection, which must be 1..4
    // Here is a case where it's faster and shorter to use data (an array) instead of
    // code (a switch statement)
    static const char *opers[] = {"add", "subtract", "multiply", "divide" };
    operation = opers[selection-1];
    
    std:: string prompt = "Enter the first number to " + operation + '\n';
    num1 = getNum(prompt.c_str(), std::numeric_limits<int>::min(), std::numeric_limits<int>::max());
    prompt = "Enter the second number to " + operation + '\n';
    num2 = getNum(prompt.c_str(), std::numeric_limits<int>::min(), std::numeric_limits<int>::max());
}

void
Calculator::calculate()
{
    switch (selection) {
    case 1:
	result = num1 + num2;
	break;

    case 2:
	result = num1 - num2;
	break;

    case 3:
	result = num1 * num2;
	break;

    case 4:
	if (num1 == 0 || num2 == 0) {
	    result = 0;
	} else {
	    result = num1 / num2;
	}
	break;
    }
}

void
Calculator::showResults()
{
    std::cout << "\n Your answer is " << result;
    std::cout << std::endl;
}

#include <iostream>
#include <string>
#include <limits>

int
main()
{
    int selection;
    Calculator calc;
    bool again = true;
    //GuessNumGame GuessNum; // Refactoring
    do {
	selection = getNum("\n Press \n 1 Calculator \n 2 Guess the Number ------------- Currently Inoperable \n 3 Exit \n Choice:",
			   1, 4);

	switch (selection) {
	case 1:
	    calc.selectOperation();
	    calc.getNums();
	    calc.calculate();
	    calc.showResults();
	    break;
	case 2:
	    //GuessNum.GuessingNumbers(); //Refactoring
	    break;
	case 3:
	    again = false;
	    break;
	default:
	    break;
	}

    } while (again);

    std::cin.ignore(std::numeric_limits < std::streamsize >::max(), '\n');
    std::cout << "\n\n Press Enter to continue: ";
    std::cin.get();

}

Handy Andy (5051)
Hello Ineff,

When I split the files I ended up with this:

Main:
1
2
3
4
5
6
7
8
9
10
 
#include <cctype>   // <--- Added. For std::tolower() and std::toupper();
#include <iostream>
#include <limits>   // <--- Added.
#include <string>

#include <ctime>    // <--- Added.
#include <cstdlib>  // <--- Added.

#include "Calculaton.hpp"
#include "GuessNumbers.h"


Calculation.hpp:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
 
#ifndef Calculator_H
#define Calculator_H

class Calculator
{
	//private:
	std::string operation;
	int selection;
	double num1, num2, result;

public:
	void selectOperation();

	void getNums();

	void doCalculation();

	void showResults(double results);
};

#endif


Calculation.cpp:
1
2
3
4
 
#include <iostream>
#include <string>

#include "Calculaton.hpp"

I messed up the name "Caalculation" a bit, but you get the ides. I used the ".hpp" extension to make it different from what you started with.

When compiled using VS2017 I had no problems or errors. This should work with other compilers.

By including the header files in <>s before any header files in ""s. The header files that come before the files in ""s will be loaded first and cover anything in the header files in ""s. As I understand it and this has worked for me so far.

For the guessing number part I did not see any fundamental problems with that code. I works. The only problem I found is with "numOfGuesses". Each time you call this part of the program "numOfGuesses" is increased by one. This works the first time it is run, but the second time it just keeps adding to the variable giving the wrong result. Before the closing brace of the function I added the line numOfGuesses = 0;
to reset this variable. You could also do this at the beginning of the function before the do/while loop. It makes no real difference where you do this just that you do.

Hope that helps,

Andy
MikeyBoy (5631)
By including the header files in <>s before any header files in ""s. The header files that come before the files in ""s will be loaded first and cover anything in the header files in ""s. As I understand it and this has worked for me so far.

That's not a good way to cope with nested dependencies. Expecting the code that includes "calculator.h" to also have to include a bunch of other stuff first, is confusing and unhelpful.

A header file should include all the other header files that are needed for the code in the header file to compile.

That way, the user of a header file can simply include it without having to worry about breaking their code due to missing dependencies.

And you've been here long enough to know that "it seems to work for me" doesn't mean the code is good, or even right, so knock that off.
Topic archived. No new replies allowed.