C++ calculator using classes

Question

This is my first time using classes objects and functions. What can I improve on?

#include <iostream>
#include <conio.h>
using namespace std;

class functions{
public:
    void Body(){
        cout << " :: Welcome to Taylor's CALCULATOR! ::" << endl;   
    }               
    int Addition(int x, int y){
        int ans = x + y;
        return ans;
    }       
    int Subtraction(int x, int y){
        int ans = x - y;
        return ans;
    }       
    int Multiplication(int x, int y){
        int ans = x * y;
        return ans;
    }       
    int Division(int x, int y){
        int ans = x / y;
        return ans;
    }
};

int main(){
int func;
int x, y;

functions key; //Object
key.Body(); //Object

cout << "What function do you want to use? " << endl;
cout << "1 - Addition " << endl;
cout << "2 - Subtraction " << endl;
cout << "3 - Multiplication " << endl;
cout << "4 - Division " << endl;
cout << "Input: " << endl;

cin >> func;
cout << endl;

switch(func){

    case 1: //Addition
        cout << "**ADDITION**" << endl;
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " + " << y << " = "; 
        cout << key.Addition(x, y);
        break;          
    case 2: //Subtraction
        cout << "**SUBTRACTION**" << endl;  
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " - " << y << " = ";
        cout << key.Subtraction(x, y);
        break;              
    case 3: //Multiplication
        cout << "**MULTIPLICATION**" << endl;   
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " x " << y << " = ";
        cout << key.Multiplication(x, y);
        break;      
    case 4: //Division
        cout << "**DIVISION**" << endl; 
        cout << "Please enter first number: " << endl;
        cin >> x;
        cout << "Please enter second number: " << endl;
        cin >> y;
        cout << x << " / " << y << " = ";
        cout << key.Division(x, y);
        break;
    default:
        cout << "Invalid Input...";
        break;
}

}

Answer 1

Unless you'll be maintaining an accumulated number for multiple operations, this may not be the best use of classes. For your code, you'd get the same effect by just performing these calculations within the switch statement (something similar to this). Right now, you're just using a class to contain similar functions, including a trivial output function.

Some additional notes:

• Try not to use using namespace std.
• Indent everything within main() as well. Not doing so may make it hard to tell that the code is being contained within something.
• You don't need <conio.h> here, so just remove it.
• It may be useful to prevent the user from dividing by 0, which is undefined behavior.

Answer 2

Repetitive block of code:

    cout << "Please enter first number: " << endl;
    cin >> x;
    cout << "Please enter second number: " << endl;
    cin >> y;

Move it outside switch or to another method. e.g. getNumbers(int& x, int& y)

You don't need to store the answer, just return the results:

return x+y;
return x-y;
etc ...

I'm aware it's just a learning example but to be honest the usage of class object here is completely pointless.

There is nothing shared between your operations that could justify the object creation.

Answer 3

The first thing I notice is that you ask for numbers 1 and 2 in every case statement. You know will always need two numbers, why not move that outside and code it once?

Answer 4

Others have already pointed out that this is a contrived example, but no one has mentioned this angle (which actually prompted me to make my first post here...)

For me it's a bit "brittle" because your main's menu is linked to the operations in the class. If you modify the functions class to add some more operations you also have to modify main() to put them in the menu and get the parameters to pass through.

One approach would be to let functions print the menu - but then it also needs to map responses to functions, so then you might say make it get the parameters as well, so you run the risk of creating a "ball of mud".

Answer 5

I'm still on the C++ learning curve but a few things I would change here is:

• Read x and y only once (using C style):

  int x, y;    
  scanf("%d %d", &x, &y);
  

  or

  int x, y;    
  cin >> x; cin >> y;
  

  You can call any operation on these values.

• Rather than switch with 1, 2 ... I would switch on characters e.g. +, - , *, / etc.

• I would change the class name to calculator as opposed to functions.

Answer 6

In addition to what Jamal said:

• Always check whether std::cin >> x succeeds.
• Don't output trailing spaces before a line break, they are useless.

For the remaining issues you can pick any beginner C++ question here, the answers are almost always the same.

Answer 7

In fact you can basically get rid of the switch block altogether, by using a map to store a pointer the appropriate function with the character of the operation as the key. Now if you make the menu choice the operation characters, just get the 2 operands and run the appropriate function. Here's a little snippet that demonstrates how to set up and use the map:

#include <iostream>
#include <map>

using namespace std;

typedef double(*OperatorFunction)(double a, double b);
typedef map<char, OperatorFunction> op_functions;

double AddFunction(double a, double b)
{
    //Add code here
}
double MinusFunction(double a, double b)
{
    //Add code here
}
double TimesFunction(double a, double b)
{
    //Add code here
}
double DivideFunction(double a, double b)
{
    //Add code here
}
int main()
{
    op_functions calcs;
    calcs['+']= &AddFunction;
    calcs['-']= &MinusFunction;
    calcs['*']= &TimesFunction;
    calcs['/'], &DivideFunction;

    cout << calcs['+'](6, 8);
    return 0;
}

Answer 8

There are some good answers here. My two cents, keeping my suggestions small and simple regarding the class is:

class Calculator{ //Classes start with capital letters.
public:

    void message(){//'Body' is vague, 'message' isn't.
        std::cout << " :: Welcome to Taylor's CALCULATOR! ::" << std::endl;
    }

    double addition(const double & x, const double & y){ //Pass by reference to reduce copies.
        return x + y; // No need to creat new variables, just return a result.
    }

    double subtraction(const double & x, const double & y){ //No need to capitalize methods of a class.
        return x-y;
    }

    double multiplication(const double & x, const double & y){ //Passed values aren't mutable, so declare them const.
        return x * y;
    }

    double division(const double & x, const double & y){ //Return values are mutable, so don't declare them const.
        if(y==0){
            std::cout << "There was a divide-by-zero error in the division function.\n Don't divide by zero please." << std::end;
            return 0;
        }
        return x / y;
    }
};

Double may be better suited in case a result requires floating point.