Calculator with four arithmetic operations

Question

#include <iostream>

using namespace std;

class Math
{
private:
    int answer;
public:
    /*void getAn()
    {
        cout << answer;
    }*/
    int add(int x, int y)
{
    cout << "Add two numbers:\n";
    cin >> x;
    cin >> y;
    answer = x + y;
    return answer;
}
    int sub(int x, int y)
{
    cout << "Subtract one number from the other:\n";
    cin >> x;
    cin >> y;
    answer = x - y;
    return answer;
}
    int multi(int x, int y)
{
    cout << "Multiply two numbers:\n";
    cin >> x;
    cin >> y;
    answer = x * y;
    return answer;
}
    int divi(int x, int y)
{
    cout << "Divide two numbers:\n";
    cin >> x;
    cin >> y;
    answer = x / y;
    return answer;
}
};

int main()
{
    int choice;
    int x,y;
    int learning = 1;
    Math doMath;
    cout << "This is a calculator!\n" << endl;
    while(learning==1)
    {
        //doMath.getAn();
        cout << endl;
        cout << "Menu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n" << endl;
        cout << "Choose your option.\n";
        cin >> choice;
        if(choice==1)
        {
            cout << doMath.add(x,y) << endl;
        }else if(choice==2)
        {
            cout << doMath.sub(x,y) << endl;
        }else if(choice==3)
        {
            cout << doMath.multi(x,y) << endl;
        }else if(choice==4)
        {
            cout << doMath.divi(x,y) << endl;
        }else if(choice==5)
        {
            learning = 0;
        }else
        {
            cout << "THAT WASN'T A CHOICE, YOU LITTLE SHIT!!!" << endl;
            learning = 0;
        }
    }
    return 0;
}

Answer 1

Do not insult users

"THAT WASN'T A CHOICE, YOU LITTLE SHIT!!!" is absolutely not acceptable.

• You are repeating yourself a lot, in programming you should avoid as much as possible code repetition as it leads to bugs and is not easily reusable.

  Instead do this:

  template<typename Function>
  int do_operation(int x, int y,Function operation,string message) {
      cout << message
      cin >> x;
      cin >> y;
      answer = operation(x,y);
      return answer;
  }
  

  You can then do:

  int add(int x,int y) {
      return do_operation(x, y, std::plus<int>(), "Add two numbers:\n");
  }
  

• I see many many if and elif, it is better to use a switch statement. You may even use an array but perhaps then it would be overkill.

• learning is quite puzzling as a name. A more standard running is more intuitive.

• //doMath.getAn(); and

  /*void getAn()
  {
      cout << answer;
  }*/
  

  are really puzzling. Either uncomment them or delete them because commented out code confuses the readers.

• Use constants for your strings and put them at the top. You can modify them in a simpler and faster way.

  static const string Menu = "Menu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n"
  static const string ErrorMessage = "Invalid choice, enter a valid number."
  

  and then

  cout << Menu << endl;
  cout << "Choose your option.\n";
  

  and

  else {
      cout << ErrorMessage << endl;
      running = 0;
  }
  

• Do not use while(learning==1) because it is redundant and weird, use the simpler while(learning) (or even better while(running))

• Do not use using namespace std;, if you are curious to know why, look here

• Put the first brace on the same line of the function title and indent the function body.

  int add(int x, int y) {
      cout << "Add two numbers:\n";
      cin >> x;
      cin >> y;
      answer = x + y;
      return answer;
  }
  

Answer 2

You don't need to pass x and y into your functions. You are passing everything by value, so you might as well declare them inside the functions. The versions outside the functions are never used. If you use Caridorc's method, you could just

template<typename Function>
int do_operation(Function operation,string message) {
    int x, y;
    cout << message
    cin >> x;
    cin >> y;
    answer = operation(x,y);
    return answer;
}

Your use of endl is a bit odd.

cout << "This is a calculator!\n" << endl;
    cout << endl;
    cout << "Menu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n" << endl;
    cout << "Choose your option.\n";

You get two things from std::endl. First, it issues a line feed in the appropriate fashion for that system. Second, it causes the output buffer to flush. I.e. it takes everything that is currently in the buffer and makes it output. Sometimes people will tell you to use a \n rather than an std::endl to avoid that flush. If you were going to do that in your code, you'd say

std::cout << "This is a calculator!\n\n";
    std::cout << "\nMenu:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n5) Quit\n\n";
    std::cout << "Choose your option.\n" << std::endl;

That does one flush at the end of the output (and you may not need that, as the std::cin will also force a flush).

Math doMath;

Objects are not verbs. It is not doing anything, so give it a noun name. E.g. math or calculator.

int learning = 1;
while(learning==1)
        learning = 0;

As Caridorc noted, learning is a bad name here. Another issue is that learning only has two possible values. This suggests to me that it is either true or false, so use a true/false variable:

bool isDone = false;
while ( ! isDone )
        isDone = true;

That's much clearer about what it's doing, but the truth is that you don't even need to do that in this case.

while (true)
        break;

That's sufficient for your needs. No extra variable needed. Loop forever until explicitly told to stop by the break; statement. Alternately, you could also use return EXIT_SUCCESS; and return EXIT_FAILURE; instead of break; in your code. You might have to do an #include <cstdlib> to get those defined.

return 0;

You don't actually need to return anything in this case. Just let it fall through and the compiler will automatically return the appropriate value.