Calculator

arcadiu (82)
Ok so this is my noobie calculator. This is not to show off my skills (the few that i have) but I was wondering if you could evaluate and analyse the code and tell me where i can improve
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
 
#include <iostream>
using namespace std;

int subtractionOne;
int subtractionTwo;
int subtractionTotal;
int additionOne;
int additionTwo;
int additionTotal;
int multiplicationOne;
int multiplicationTwo;
int multiplicationTotal;
int divisionOne;
int divisionTwo;
int divisionTotal;

int division() {
	
	cin >> divisionOne >> divisionTwo;
	int divisionTotal = divisionOne / divisionTwo;
	
	cout << divisionTotal;
	
	return divisionTotal;
	
}
	
	
int multiplication() {
	cin >> multiplicationOne >> multiplicationTwo;
	int multiplicationTotal = multiplicationOne * multiplicationTwo;
	
	cout << multiplicationTotal;
	
	return multiplicationTotal;
	
}

int addition() {
	
	cin >> additionOne >> additionTwo;
	int additionTotal = additionOne + additionTwo;
	
	cout << additionTotal;
	
	return additionTotal;
	
}

int subtraction() {
	
	
	cin >> subtractionOne >> subtractionTwo;
	int subtractionTotal = subtractionOne - subtractionTwo;	
	
	cout << subtractionTotal;
	
	return subtractionTotal;
}

int main () {
	int choose;	
	
	system ("clear");
	
	cout << "Press one for subtracting, two for adding, three for multiplication and four for division.";
	cin >> choose;
	
	if(choose == 1) {
		
		subtraction();
		
	}
	
	if(choose == 2) {
		
		addition();
		
	}	
	
	if(choose == 3) {
		
		multiplication();
		
	}
	
	if(choose == 4) {
		
		division();
	
	
	}
	return 0;
}

kyle11778 (192)
in your int main() you should use case instead of all the if()'s
all of your other function dont need to return anything since nothig is being passed so put only return and use void instead of int

other than that, good job
Return 0 (1282)
I don't see all of those global variables being necessary. Why do you need to keep any of the totals? Do you plan on using them later? Your program executes only once. Get rid of this:

system ("clear");
kyle11778 (192)
Return 0 is right, no need.

this is my version of yours
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
 
#include <iostream>
using namespace std;

int subtractionOne;
int subtractionTwo;
int subtractionTotal;
int additionOne;
int additionTwo;
int additionTotal;
int multiplicationOne;
int multiplicationTwo;
int multiplicationTotal;
int divisionOne;
int divisionTwo;
int divisionTotal;

void division() {
	
	cin >> divisionOne >> divisionTwo;
	divisionTotal = divisionOne / divisionTwo;
	cout << divisionTotal;
	return;
	
}
	
	
void multiplication() {
	cin >> multiplicationOne >> multiplicationTwo;
	multiplicationTotal = multiplicationOne * multiplicationTwo;
	
	cout << multiplicationTotal;
	
	return;
	
}

void addition() {
	
	cin >> additionOne >> additionTwo;
	additionTotal = additionOne + additionTwo;
	
	cout << additionTotal;
	
	return;
	
}

void subtraction()
{
	cin >> subtractionOne >> subtractionTwo;
	subtractionTotal = subtractionOne - subtractionTwo;	
	
	cout << subtractionTotal;
	
	return;
}

int main () {
	int choose;	
	
	cout << "Press one for subtracting,\n two for adding,\n three for multiplication,\n and four for division.";
	cin >> choose;
	switch(choose)
	{
          case 1:
               subtraction();
               break;
          case 2:
               addition();
               break;
          case 3:
               multiplication();
               break;
          case 4:
               division();
    }
	system("pause");
	return 0;
}

hope it helps
Return 0 (1282)
Kyle1178....You removed one system call just to replace it with another... Why are there return statements at the end of every function? I want you to think about the design of this calculator. Unless you are going to use the totals or individual numbers for something later, there is really no need to keep them in memory.

Here's what I think your next steps should be. Address the issues I mentioned and try the following:

1. Rewrite the program using only two variables.
2. Add divide by zero exception handling in your division function.
3. Decide on whether you need to return a value, or just output it to the console window and rewrite the associated code.

This will be good practice for you.
kyle11778 (192)
me or arcadiu hehehe
Return 0 (1282)
both :)
kyle11778 (192)
rude lol
i know all of the points you are making i just didn't want to change too much.
Return 0 (1282)
Don't take offense. If you know this stuff don't worry about it.
rvbman47 (16)
You should make the menu system better, this is what I normally 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
 
#include<windows.h>

void setcolor(unsigned short color)
{
    HANDLE hcon = GetStdHandle(STD_OUTPUT_HANDLE);
    SetConsoleTextAttribute(hcon,color);
}

int mainmenu()
{
    int input=1;
    char key='a';
    while(key != 13)
    {
        system("CLS");
        cout << "_-{Main Menu}-_";
        if(input==1)
            setcolor(135);
        cout << "\nSubtraction";
        setcolor(7);
        if(input==2)
            setcolor(135);
        cout << "\nAddition";
        setcolor(7);
        if(input==3)
            setcolor(135);
        cout << "\nMultiplication";
        setcolor(7);
        if(input==4)
            setcolor(135);
        cout << "\nDivision";
        setcolor(7);
        if(input==5)
            setcolor(135);
        cout << "\nQuit";
        setcolor(7);
        key=getch();
        if(key == 'P' && input<5)
            input++;
        if(key == 'H' && input>1)
            input--;
    }
    return input;
}

You need #include<windows.h> for void setcolor() to work.

Ascii can be strange at times; the up arrow key is
à and P, and translates to 224 and 72, the down arrow key is
à and H, and translates to 224 and 80. Which is why this section works:

1
2
3
4
 
if(key == 'P' && input<5)
    input++;
if(key == 'H' && input>1)
    input--;
Last edited on
arcadiu (82)
Sorry for my noobieness but I am not onto advanced stuff. I only know really up to functions. Someone said above use only 2 variables... how is this possible?

Sorry to bring up the thread again but I suppose I need help to improve my skills.
arcadiu (82)
Sorry for my noobieness but I am not onto advanced stuff. I only know really up to functions. Someone said above use only 2 variables... how is this possible?

Sorry to bring up the thread again but I suppose I need help to improve my skills.
rvbman47 (16)
what if you just used choose=getch(); instead of cin >> choose; so that you don't have to press enter after you type in the number.
jsmith (5804)
There is no need to have all these variables:

int subtractionOne;
int subtractionTwo;
int subtractionTotal;
int additionOne;
int additionTwo;
int additionTotal;
int multiplicationOne;
int multiplicationTwo;
int multiplicationTotal;
int divisionOne;
int divisionTwo;
int divisionTotal;

The point is that inside subtraction(), you need to read in two integers. You could read them
into variables called "first_number" and "second_number". You could then modify every other function to use "first_number" and "second_number" instead of different variables.

You could do the same thing with all of the "...Total" variables. Just have one variable called "result" that is used by every function.

Then, since you don't ever need to keep around the numbers the user entered and the result of the calculation once the function returns, you could make these local variables inside the functions instead of global variables.
Topic archived. No new replies allowed.