C++

Forum

 
 
Need Efficiency/Simplest Code

Chahunda15 (13)
Hello, I am learning how to code in C++ and one of my assignments is to make a calculator. Before I turn this in, is there anything that I am doing wrong or
could I make this more efficient /easier. Thank you in advance for your help!
I am using Visual Studio Professional 2013.

P.S. - I was told not to use the "using namespace std;" for this assignment.

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
 
  // C++ConsolePractice.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <iostream>
#include <string>

int main()
{
	restart:
	std::cout << "Enter a number:    " << std::flush;
	float a;
	std::cin >> a;

	std::cout << "Enter an operator: " << std::flush;
	std::string b;
	std::cin >> b;

	std::cout << "Enter a number:    " << std::flush;
	float c;
	std::cin >> c;

	if (b == "+")
	{
		std::cout << a << " + " << c << " = " << a + c << std::endl;
		std::cout << std::endl;
		goto restart;
	}

	if (b == "-")
	{
		std::cout << a << " - " << c << " = " << a - c << std::endl;
		std::cout << std::endl;
		goto restart;
	}

	if (b == "/")
	{
		std::cout << a << " / " << c << " = " << a / c << std::endl;
		std::cout << std::endl;
		goto restart;
	}

	if (b == "*")
	{
		std::cout << a << " * " << c << " = " << a * c << std::endl;
		std::cout << std::endl;
		goto restart;
	}

	return 0;
}
Last edited on
Ch1156 (1999)
well for starters, dont use goto :), if you want your program to loop you would use a while loop:

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
 
#include <iostream>
#include <string>

int main()
{
    bool loopEnd = false;

    while(!loopEnd)
    {
        std::cout << "Enter a number:    " << std::endl;
        float a;
        std::cin >> a;

        if(a == -1)
        {
            loopEnd = true;
        }
        else
        {
            std::cout << "Enter an operator: " << std::endl;
            char b;
            std::cin >> b;

            std::cout << "Enter a number:    " << std::endl;
            float c;
            std::cin >> c;

            switch(b)
            {
                case '+':
                    std::cout << a << " + " << c << " = " << a + c << std::endl;
                    std::cout << std::endl;
                    break;

                case '-':
                    std::cout << a << " - " << c << " = " << a - c << std::endl;
                    std::cout << std::endl;
                    break;

                case '/':
                    std::cout << a << " / " << c << " = " << a / c << std::endl;
                    std::cout << std::endl;
                    break;

                case '*':
                    std::cout << a << " * " << c << " = " << a * c << std::endl;
                    std::cout << std::endl;
                    break;

                default:
                    std::cout << "Error" << std::endl;
                    break;

            }
        }
    }

	return 0;
}


goto is dangerous and makes code ugly and hard to read if you have too many of them.
Last edited on
shawnlau (79)
I'm guessing you could make it more c++ like. A switch statement would be more optimized than if statements.

I'm going back to old days of C and ansi C. An if statement requires pushing two values onto the stack. A switch statement turn into a jump statement on the assembly level.
Last edited on
Ch1156 (1999)
ah yes, what shawnlau said is a good suggestion as well, I have updated my code to a switch statement instead of if's.

NOTE: switch statements cannot take strings, only chars, ints, floats, doubles and bools

Also with your cin statements you could do this as well:

1
2
3
4
 
std::cout << "Enter an operator, press enter and then enter a number: " << std::endl;
char b;
float c;
std::cin >> b >> c;
Last edited on
booradley60 (896)
Any time you see repeated code, there's probably an opportunity to generalize it and stick it in one place. See the output statements here:
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
 
#include <iostream>
#include <string>

int main()
{
    while (true)
    {
        std::cout << "Enter a number:    " << std::endl;
        float firstOperand;
        std::cin >> firstOperand;

        if (firstOperand == -1)
        {
            break;
        }

        std::cout << "Enter an operator: " << std::endl;
        std::string theOperator;
        std::cin >> theOperator;

        std::cout << "Enter a number:    " << std::endl;
        float secondOperand;
        std::cin >> secondOperand;

        float answer = 0.0;
        if (theOperator == "+")
        {
            answer = firstOperand + secondOperand;
        }
        else if (theOperator == "-")
        {
            answer = firstOperand - secondOperand;
        }
        else if (theOperator == "/")
        {
            answer = firstOperand / secondOperand; //should make sure that secondOperand isn't zero
        }
        else if (theOperator == "*")
        {
            answer = firstOperand * secondOperand;
        }

        std::cout << firstOperand << ' ' << theOperator << ' ' << secondOperand << " = " << answer << std::endl;
        std::cout << std::endl;
    }
    return 0;
}


I also changed some of the variable names. This makes the code easier to read. It takes more typing, but code is generally read more than it is written.

Also, by putting a break in the if (firstOperand == -1) statement, the need for a loop stopping variable is eliminated, and you don't have to put a bunch of code into an else block, saving a level of indentation.
Last edited on
CodeWriter (286)
Maybe let the user know how to exit?
1
2
3
4
5
6
7
8
9
10
11
 
int main()
{  
    std::string reply;
    do{

        // do stuff

        std::cout << "Again y/n?  " << std::flush;
        std::cin >> reply;
    }while (reply == "y");
}

Ch1156 (1999)
I agree with booradley60, when you have variables with ambiguous names, it will make it harder to understand when you come back to it, weeks, months or even years later.
Last edited on
Chahunda15 (13)
For Ch1156 - I haven't learned about switch statements but thats seems to look better!
Also yes I will try the different way of cin statements.
I don't understand the point of the if statement on line 14

For booradley60 - I like your name(to kill a mockingbird). Also yes the while loop does look
promising but I don't understand the point of the if statement on line 12
Ch1156 (1999)
the if statement on line 14 basically is saying: "If you type in -1, the program will end"

although i did not program a message that informs the user of this, it would be a good idea to do so.

Switch statements are quite easy to learn and are very helpful. basicaly whenever you have more than 2 or 3 if statements, see if you can re write that code into a switch statement.
Last edited on
Chahunda15 (13)
Is this better?

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
 
#include <iostream>
#include <string>

int main()
{
	std::string reply;
	do
	{
		std::cout << "Enter a number:" << std::endl;
		float a;
		std::cin >> a;

		std::cout << "Enter an operator:" << std::endl;
		char b;
		std::cin >> b;

		std::cout << "Enter a number:" << std::endl;
		float c;
		std::cin >> c;

		switch (b)
		{
			case '+':
				std::cout << a << " + " << c << " = " << a + c << std::endl;
				std::cout << std::endl;
				break;

			case '-':
				std::cout << a << " - " << c << " = " << a - c << std::endl;
				std::cout << std::endl;
				break;

			case '/':
				std::cout << a << " / " << c << " = " << a / c << std::endl;
				std::cout << std::endl;
				break;

			case '*':
				std::cout << a << " * " << c << " = " << a * c << std::endl;
				std::cout << std::endl;
				break;

			default:
				std::cout << "Error" << std::endl;
				break;

		}

		std::cout << "Again y/n?  " << std::flush;
		std::cin >> reply;
		std::cout << std::endl;
	} while (reply == "y");

	return 0;
}
Last edited on
CodeWriter (286)
Remember that names (labels) can be self commenting.
Thus instead of using the label b for an operator maybe use "op" or "opr" or "oper".
You can't use "operator" because it is a reserved word.
And instead of a and c you might use num1 and num2.
Last edited on
shawnlau (79)
I think it looks much more like c++ now. The first looked a lot like BASIC. My problem is my code looks more like c than c++, but thats where I learned.
Last edited on
Ch1156 (1999)
Thats much better :)
CodeWriter (286)

@shawnlau wrote:
My problem is my code looks more like c than c++, but thats where I learned.


I guess you would have to implement a calculator class.
http://www.codenirvana.net/2013/06/simple-calculator-program-in-c.html
Topic archived. No new replies allowed.