C++ Simple Calculator

Question

I'm very new to C++ and am trying to code simple programs to test my knowledge on the very basics of this language. Yesterday, I created a very simple calculator script which allows you to add, subtract, multiply, and divide and set of two numbers. While it is still very basic (e.g. only calculating two numbers), I plan on adding more functionality and improving the overall code-base to be not only more efficient, but a more pleasing experience to review. Remember, I am still VERY new to this and need some guidance (for example, I used goto(label) to escape nested loops, there's probably a much better way to do this) on how I can improve this code. Thanks.

main.cpp

#include <iostream>
#define log(x) std::cout << x;
#define logl(x) std::cout << x << std::endl;
int main() {
 int choice, choice2;
 float first, second;
 logl("-----------------------");
 logl("schmobbing's calculator");
 logl("-----------------------");
 start:
 std::cout << "" << std::endl;
 logl("Choose one of the following calculation options.");
 log("[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ");
 std::cin >> choice;
 while (true) {
 switch (choice) {
 case 1:
 logl("");
 logl("You've chosen addition.");
 break;
 case 2:
 logl("");
 logl("You've chosen subtraction.");
 break;
 case 3:
 logl("");
 logl("You've chosen multiplication.");
 break;
 case 4:
 logl("");
 logl("You've chosen division.");
 break;
 default:
 logl("");
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("You've chosen an invalid operator, please try again.");
 log("[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ");
 std::cin >> choice;
 continue;
 } else {
 logl("You've chosen an invalid operator, please try again.");
 log("[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ");
 std::cin >> choice;
 continue;
 }
 }
 break;
 }
 while (true) {
 switch (choice) {
 case 1:
 while (true) {
 log("Enter your first number: ");
 std::cin >> first;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 while (true) {
 log("Enter your second number: ");
 std::cin >> second;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 logl("");
 std::cout << "The sum of the given numbers is " << first + second << "." << std::endl;
 while (true) {
 log("Would you like to calculate for a different result? [1] Yes or [2] No: ")
 std::cin >> choice2;
 switch (choice2) {
 case 1:
 goto start;
 case 2:
 goto end;
 default:
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 } else {
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 }
 }
 }
 break;
 case 2:
 while (true) {
 log("Enter your first number: ");
 std::cin >> first;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 while (true) {
 log("Enter your second number: ");
 std::cin >> second;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 logl("");
 std::cout << "The difference of the given numbers is " << first - second << "." << std::endl;
 while (true) {
 log("Would you like to calculate for a different result? [1] Yes or [2] No: ")
 std::cin >> choice2;
 switch (choice2) {
 case 1:
 goto start;
 case 2:
 goto end;
 default:
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 } else {
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 }
 }
 }
 break;
 case 3:
 while (true) {
 log("Enter your first number: ");
 std::cin >> first;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 while (true) {
 log("Enter your second number: ");
 std::cin >> second;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 logl("");
 std::cout << "The product of the given numbers is " << first * second << "." << std::endl;
 while (true) {
 log("Would you like to calculate for a different result? [1] Yes or [2] No: ")
 std::cin >> choice2;
 switch (choice2) {
 case 1:
 goto start;
 case 2:
 goto end;
 default:
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 } else {
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 }
 }
 }
 break;
 case 4:
 while (true) {
 log("Enter your first number: ");
 std::cin >> first;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 while (true) {
 log("Enter your second number: ");
 std::cin >> second;
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid number, please try again.");
 continue;
 }
 break;
 }
 if (second == 0) {
 logl("");
 std::cout << "The quotient of the given numbers is undefined." << std::endl;
 } else {
 logl("");
 std::cout << "The quotient of the given numbers is " << first / second << "." << std::endl;
 }
 while (true) {
 log("Would you like to calculate for a different result? [1] Yes or [2] No: ")
 std::cin >> choice2;
 switch (choice2) {
 case 1:
 goto start;
 case 2:
 goto end;
 default:
 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 } else {
 logl("");
 logl("You've chosen an invalid option, please try again.");
 break;
 }
 }
 }
 }
 end:
 break;
 }
}
```

• Pablochaches: In short words. DONT use macros. goto should be avoided, but if it is really clear what you are doing, like breaking from multiple nested loops, it is fine. The code is in general hard to read, try to split it into functions. Well, that is the biggest recomendation, learn to use functions. Things like the validation of the user input are copy paste, it is better to abstract it into a function. Just as something tho take into account when you learn abaut them. Give them only one clear purpose, and give then good names.
• user228914: @Pablochaches " DONT " use macros? Why? I think I know what you are trying to prove here but DONT makes it sound like its extremely bad to have them in your program.
• Pablochaches: @AryanParekh True, maybe I was a little exagerated. Let me correct. Use macros only when 100% necesary. For things like Logging for debug purposes they are good. Contracts in C++ can only be done, at this time, using macros. But, for things stuff like in this case, if it looks like a function make it a function. If it looks like a constant make it constexpr.
• greybeard: Don't vandalise posts. You can delete it, but consider whether there is value in at least one of the answers.

Answer 1

Welcome to C++!

---

Empty logl("")

I have seen a few them in your program. But I wasn't 100% sure as to why they keep appearing. My guess is that you are to print a newline. In that case, you should just use something called escape sequences. Specifically, '\n'

Printing '\n' Will just print a new line onto the terminal

std::cout << "Hel\nlo!";
// Output
Hel
lo!

Since you are new, another escape sequence that is often used is the \t. i.e the TAB. Using this lets you print 8 spaces onto the terminal.

std::cout << "Hel\tlo!";
// Output
Hel lo!

---

'\n' vs std::endl

My previous point must've left you wondering whether you should use '\n' or std::endl. Since both of them will perform the task, which is to print a new line.

In this case, you should use '\n'. '\n' will be faster to run since std::endl calls std::flush. Basically, std::endl flushes the output stream every time you call it. Which isn't as fast as printing '\n'.

std::cout << std::endl;
std::cout << '\n' << std::flush; // equivalent to the top

"std::endl" vs "\n"

---

Formatting output

 logl("-----------------------");
 logl("schmobbing's calculator");
 logl("-----------------------");
 start:
 std::cout << "" << std::endl;
 logl("Choose one of the following calculation options.");
 log("[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ");

How about you save this in a variable, and only print that? You can achieve this using the raw string literal.

auto Heading = R"(
 -----------------------
 schmobbing's calculator
 -----------------------
)";
auto Instructions = R"(
Choose one of the following calculation options.
[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division
)";
auto InvaildOperator = R"(
You've chosen an invalid operator!
)"
std::cout << Heading;
std::cout << Instructions;

Here if you want to make tweaks to how the instructions would look, it would be very easy.

---

Repetition

 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 logl("You've chosen an invalid operator, please try again.");
 log("[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ");
 std::cin >> choice;
 continue;
 } else {
 logl("You've chosen an invalid operator, please try again.");
 log("[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ");
 std::cin >> choice;
 continue;
 }

You have written something twice here. Both the if statements do the exact same thing, except that the first one does something extra. Therefore, keep that extra in the if statement and move everything else out.

 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 }
 logl("You've chosen an invalid operator, please try again.");
 log("[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ");
 std::cin >> choice;
 continue;

+1 if you followed my previous advice.

 if (!std::cin) {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 }
 std::cout << InvalidOperator; 
 std::cout << Instructions;
 std::cin >> choice;
 continue;

---

Use a do-while loop

Instead of having goto which many programmers don't like, use a simple do-while loop.

char PlayAgain;
do {
 // ...
 // ... 
 // According to user's input, make `PlayAgain` y or n
} while(std::tolower(PlayAgain) == 'y');

---

Functions, please

The biggest thing missing in the program are functions. You have a huge bunch of code in your main() which is very hard to read. Split your work into different functions. Follow the Single-responsibility principle.

Kainev's answer gives a good example of how the program can look with proper structure.

---

No magic numbers

case 1:
case 2:
case 3:
case 4:

This was the switch statement from your program for the operator selections. Over here 1, 2, 3, and 4 don't necessarily mean anything. They're called magic numbers. i.e Unnamed numeric constants. A better thing to do would be to use an enum

enum Operator { Add = 1, Subtract, Multiply, Divide };
case Operator::Add:
case Operator::Subtract:
//...

Subtract, Multiply, and Divide are automatically set to 2, 3, and 4 respectively. Now you don't have meaningless numbers pollution your program.

Answer 2

Macros

It is important to be very careful when using macros. A major pitfall of C++ macros is that they ignore namespaces, for example:

#include <map>
#define map
int main()
{
 std::map<int, int> my_map;
}

The macro map will completely ignore the std namespace, this results in std::map_macro_inserted_here<int, int> my_map which will cause a compiler error.

This is why it is highly recommended to name all macros in UPPER_CASE, this lets you know its a macro and helps avoid naming clashes.

I don't mind the use of a LOG macro, however it is more often used as a convenience for a more complicated logging system, rather than a single line cout.

while(true)

This isn't necessarily bad, and can be useful, however if there is a logical condition that can be used instead of true, you should preference that. It makes the intent clearer, and decreases the chance of getting stuck in the loop due to a logical error. To see this in action, look at the menu_selection function in my example code.

goto and code reuse

In general if you ever feel the need to write a goto statement, you should probably be writing a function instead.

When reading through your code, if you start to find patterns where you've written the same or even similar code in multiple places, this is also a sign you should be writing a function instead.

There are a couple of benefits for doing this including:

• clearer/more concise code, making it easier for others to read.
• Better maintainability

Example

Here is a possible refactor. It is 87 lines long, with whitespace for readability, versus the original 272 with almost no whitespace. It's definitely not perfect, but hopefully demonstrates the idea of code reuse and the benefits it has:

Update: Added enum's to eliminate "magic" numbers as suggested in Aryan's answer.

#include <iostream>
enum Operator { Add = 1, Subtract = 2, Multiply = 3, Divide = 4 };
enum ExitMenuOption { Continue = 1, Exit = 2 };
int menu_selection(const std::string& menu, int min_option, int max_option);
float get_float_input(const std::string& prompt);
void calculator(Operator operation);
int main()
{
 const std::string title_bar = "-----------------------\nschmobbing's calculator\n-----------------------";
 const std::string operation_menu = "Choose one of the following calculation options.\n[1] Addition, [2] Subtraction, [3] Multiplication, [4] Division: ";
 const std::string exit_menu = "Would you like to calculate for a different result? [1] Yes or [2] No: ";
 std::cout << title_bar << "\n";
 do
 {
 Operator operation = static_cast<Operator>(menu_selection(operation_menu, 1, 4));
 calculator(operation);
 } while (menu_selection(exit_menu, 1, 2) != ExitMenuOption::Exit);
}
int menu_selection(const std::string& menu, int min_option, int max_option)
{
 int choice = -1;
 while (choice < min_option || choice > max_option)
 {
 std::cout << menu;
 std::cin >> choice;
 if (!std::cin)
 {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 std::cout << "\nYou've chosen an invalid option, please try again.\n";
 }
 }
 std::cout << "\n";
 return choice;
}
float get_float_input(const std::string& prompt)
{
 float input;
 do
 {
 std::cout << prompt;
 std::cin >> input;
 if (std::cin)
 {
 break;
 }
 else
 {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
 std::cout << "\nYou've chosen an invalid number, please try again.";
 }
 } while (true);
 return input;
}
void calculator(Operator operation)
{
 float first = get_float_input("Enter your first number: ");
 float second = get_float_input("Enter your second number: ");
 switch (operation)
 {
 case Operator::Add:
 std::cout << "\nThe sum of the given numbers is " << first + second << ".\n";
 break;
 case Operator::Subtract:
 std::cout << "\nThe difference of the given numbers is " << first - second << ".\n";
 break;
 case Operator::Multiply:
 std::cout << "\nThe product of the given numbers is " << first * second << ".\n";
 break;
 case Operator::Divide:
 if (second == 0)
 std::cout << "\nThe quotient of the given numbers is undefined.\n";
 else
 std::cout << "\nThe quotient of the given numbers is " << first / second << ".\n";
 break;
 }
}

• Peter Cordes: Your point about macro names is especially important given that the names chosen (log and logl) are names of standard math functions! There is a std::log in <cmath>, and a full-featured calculator is likely to want it. And in most C++ implementations, <math.h> will put those functions in the global namespace. (logl is log for long double, logf is for float. log is overloaded for either or plain double, which is the normal go-to FP type if you don't need to save memory bandwidth / cache footprint with single float).
• kainev: Correct, it is not required to specifically state each value, however I wanted to make it clear what was happening. Regarding the return values: The functions task is to get the selection from the user, once they've selected a valid option we need to return that value so we can use it in the rest of our program. The variables input and choice are local variables to their respective functions. they only exist within that function. You can research variable scope to find out more information.
• user228914: @schmobbing yes the values specified are redundant. Some people like to have them, some people don't. It's completely up to you.
• user228914: @schmobbing kainev has basically created a function that takes a float number from the user while handling the exceptions. This allows him to re-use that function everywhere he would need a floating number as input from the user and he wouldn't have to write the same block of code again.