15
\$\begingroup\$

I made a calculator in C++. I wanted to ask is this code well written, understandable, and alright.

#include <iostream>
using namespace std;

int main() {
        // Declares the variable to store the asked value
        int num1, num2;
        char op;
        // Asks the user for the input of numbers and the operation
        cin >> num1;
        
        cin >> op;
        
        cin >> num2;
        // Declares a variable for the answer
        int result;
  // Handles all the conditions of the operation
  if(op == '+') {
    result = num1 + num2;
} else if(op == '-'){
    result = num1 - num2;
} else if(op == '*') {
    result = num1 * num2;
} else if(op == '/') {
    result = num1 / num2;
} else {
    cout << "Invalid values";
    cout<<endl<<endl;
}
    // Outputs the result
    cout << "Answer: ";
    cout << result;
    // Ouputs a thankyou message
    cout<<endl<<endl;
    cout << "THANKS FOR USING THIS";
    return 0;
}
\$\endgroup\$
8
  • \$\begingroup\$ You could use a switch instead of the if statements, I guess \$\endgroup\$
    – user
    Jun 11, 2020 at 14:57
  • \$\begingroup\$ @user Thanks for the reply bro but how does it make my code better is it faster than using If/else or something?g sorry If I might irritate you for a reason but I am newbie. \$\endgroup\$
    – XYBOX
    Jun 11, 2020 at 15:08
  • 6
    \$\begingroup\$ Switch can be a bit faster sometimes (although it probably won't matter in such a small program). However, I meant that it might look a bit prettier that way. Also, you don't need to apologize for being a newbie. You're not irritating at all - this site is meant for questions. \$\endgroup\$
    – user
    Jun 11, 2020 at 15:25
  • 15
    \$\begingroup\$ It is bad practice to use using namespace std;. See this SO question for more info. \$\endgroup\$ Jun 12, 2020 at 1:22
  • 4
    \$\begingroup\$ @MarquisofLorne Well the guy is a newbie so for him, this stuff is a bit complex at least the program works and I think he has done pretty well as a beginner. \$\endgroup\$ Jun 12, 2020 at 12:03

4 Answers 4

12
+100
\$\begingroup\$

I have some suggestions here:

  1. Using switch/case instead of if/else looks a little better and slightly faster as also mentioned in the comments.

  2. You could definitely work on formatting your code it can be a lot better

\$\endgroup\$
6
  • 22
    \$\begingroup\$ If switch is faster than the equivalent if, you need to get yourself a new compiler. A good optimizer does not care, and will transform either construct into the most efficient code possible, whether that's a LUT, binary search, or whatever. Therefore, do not choose between if/switch based on performance, but rather on readability. Also, strongly prefer a switch when there is an exhaustive list of possibilities, and you aim to cover them all. \$\endgroup\$ Jun 12, 2020 at 2:19
  • \$\begingroup\$ @CodyGray Ok alright \$\endgroup\$ Jun 12, 2020 at 8:06
  • \$\begingroup\$ @CodyGray though I use the built-in compiler in visual studio but maybe my PC is slow. \$\endgroup\$ Jun 12, 2020 at 9:12
  • \$\begingroup\$ Unless you extract the computation of the result into a separate function, I advise against using switch here because the break statements add a lot of visual overhead, and it's too easy to forget a break. Sure, at one point you have to learn it, but in this early phase of programming just stay with the basics. \$\endgroup\$ Jun 13, 2020 at 5:17
  • \$\begingroup\$ @RolandIllig Yes you are totally correct. \$\endgroup\$ Jun 13, 2020 at 6:43
10
\$\begingroup\$

A good next step in your code would be to test edge cases and account for them. A couple things you could improve:

  1. Your code does not handle division by zero. If someone puts in 1 / 0 your program will crash. You can check for this and give the user a nice error message

  2. Your code does not handle integer overflow. You'll get interesting results if you try to do 2000000000 * 2000000000 because an int can only hold so large of a value.

\$\endgroup\$
2
  • \$\begingroup\$ I thought div by 0 is UB? But maybe that's only C? Still should check it of course (though maybe not needed for floating point arithmetic). \$\endgroup\$
    – Pryftan
    Jun 14, 2020 at 15:58
  • \$\begingroup\$ @Pryftan It is UB but on any realistic platform today it will most likely crash. With floating-point numbers it's usually positive infinity. \$\endgroup\$
    – S.S. Anne
    Jun 14, 2020 at 20:58
5
\$\begingroup\$

Switch/Case over if/else

I think it would be great if you use switch/case instead of if/else it also makes you code look neat. Something like this

switch(op)
    {
      case '+': result = num1 + num2;    break;
      case '-': result = num1 - num2;    break;
      case '*': result = num1 * num2;    break;
      case '/': result = num1 / num2;    break;
      default:
          cout << "Please enter valid operation" ;
    }

Moreover, using switch/cases over if/else improves computational time when doing multiple iterations.

Add an OFF button

Also, the physical calculators that we use are always 'ON' unless they are turned off by pressing an 'OFF' button (or get turned off if no key is pressed for time \$t\$). You can also add that feature.

Instruction: type of input expected form user.

Say, a friend of yours wants to run the code, or you are an app developer and your client do not know anything about programming, and he just double-clicks on calculator.exe file, it would be great if you can also mention a short note when the program begins telling what logical operations and input format is expected from the user.

Showing error message

When someone types in num1 / 0. You can either show the error message

cout << DIVIDE_BY_ZERO_ERROR << endl;

Or simply say the answer in Infinity. And its value is

#include <limits>
// ...

double a = std::numeric_limits<double>::infinity();

You do not need to show this value, just in case if you want to take this code further and add some more features like 'memory' like our typical calculators. Note: this is not real infinity! It just a number such that \$a>b\,\,\forall b\$. There is more about Handling the Divide by Zero Exception in C++. Perhaps, I have given only a short glimpse sticking with minimal code policy.

\$\endgroup\$
10
  • 1
    \$\begingroup\$ Oo Thank you very much for this wonderful example. \$\endgroup\$
    – XYBOX
    Jun 11, 2020 at 16:04
  • \$\begingroup\$ What do you mean by “this is not real infinity”? It is a bit configuration that means infinity, and is treated as such by all floating-point operations. Just like NaN is a bit configuration that indicates NaN, and 0 is a bit configuration that indicates 0, and 2.25 is a bit configuration that indicates 2.25, ... \$\endgroup\$ Jun 13, 2020 at 19:23
  • \$\begingroup\$ @CrisLuengo some infinities are bigger than other infinities! The infinity defined here is same as other infinities. Check yourself if(inf*inf > inf) std::cout << "Hell yeah! Some infinities are greater than other infinities \n"; This is the reason I was critical about that note. \$\endgroup\$ Jun 14, 2020 at 0:20
  • \$\begingroup\$ @KartikChhajed: if that tests positive, it is a compiler-dependent result, maybe caused by some optimization? IEEE defines exactly one representation for infinity, and infinity should compare equal to itself. \$\endgroup\$ Jun 14, 2020 at 0:31
  • 1
    \$\begingroup\$ Ah, I see what you mean now. Some set that has an infinite number more elements than another infinite set, like happens with real and natural numbers. But even in that case, both sets have an infinite number of elements, there’s only one symbol for infinity in mathematics. Given x x = y, if you fill out x = infty, then y = infty as well. There is no such notion as a larger infinity. The only awkward thing in IEEE floats is that infty==infty, whereas they should really not be comparable. But it’s convenient being able to compare them, and hence they’re comparable. \$\endgroup\$ Jun 14, 2020 at 1:15
4
\$\begingroup\$

Prefer switch over else ifs

Switch will make your code easier to read.

The variable result could be uninitialized

Consider situation where user types in incorrect operator. In this case the result is uninitialized and your calculator crashes.

Division by zero crashes

Prefer \n over endl

The endl forces the stream to flush. Instead of

cout << "Answer: ";
cout << result;
// Outputs a thankyou message
cout << endl << endl;
cout << "THANKS FOR USING THIS";

I would suggest something like

std::cout << "Answer: " << result 
    << "\n\nTHANKS FOR USING THIS\n";
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.