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;
}
}

• Elias Benevedes: Posting as a comment because it's not enough to substantiate an answer; make sure that in division if they put in 0 as the second value it will catch that or you will run into a divide by 0 error.
• Winther: The division function as defined is 'integer division', for example it returns the result 5 / 2 = 2 and not 2.5. If this is what you want then fine, otherwise you should change the type of this function to double.
• Konrad Rudolph: This may sound snappy — and for that I apologise. But, in all honesty, the correct answer to your question, "what can I improve on?" is, "don’t use classes here". Your class actually serves no purpose (except to group functions — so use a namespace instead!).
• Ron Beyer: @KonradRudolph If this were real-world I'd whole-heartedly agree, however it is pretty clear this is a learning exercise. In the real world there is almost never a case that you would want to write an Addition(x,y) method versus just writing x+y in the code itself. If you want to boil this down, the answer would be "don't reinvent the wheel" and to just use x+y, x-y, etc in the switch.

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.

• user20300: there's one thing shared - use semantics of the methods; IMO he could (or rather, should) turn those methods static, and treat this class as an utility one. Otherwise, I wholeheartedly agree.
• woockashek: Then it should be rather a namespace, not a class full of static methods. Less typing and less confusion.
• user20300: not necessarily; there's a distinct difference between the usual use of namespaces and the usual use of Utility Class pattern - in most real-life situations I've seen, C++ namespace is a "top level" container (e.g. one namespace for a piece of software), similar to e.g. package in Java or namespace in C#; as such, e.g., you'd have namespace mathtools { class Calculator {} }; I'm not saying it's the best choice in this particular case, but a) it's a tutorial program, so it should, well, tutor, b) it's the general rule of thumb for actual software I've seen.

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.

• WorldSEnder: scanf in c++? I'd advocate against it. The proposed name calculator is very misleading without also pulling logic into the class, it's more like a namespace math right now
• Tolani: @WorldSEnder scanf is a C style. I will update my answer

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;
}

• forsvarir: -1 is a valid answer, why are you returning it for divide by zero?
• user33306: @forsvarir - The code I copied this from used that as an error code. It can easily be changed to throw an exception instead or trapped before the function is used. Either way the concept being demonstrated is valid.
• user33306: Since some people don't have enough intelligence to read the answer and see that the purpose isn't to deliver perfect functions but to demonstrated a concept, none of the functions have bodies.
• Juho: By the way, no need to define those (boring) functions yourself. Just use std::plus etc. which are already defined. In general, I like your answer.

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.

• forsvarir: An int is a basic type, that is typically the same size as a reference, what is it you are gaining with this approach? Is recommending returning a fixed random answer rather than throwing an exception for divide by zero really setting the beginner off in a good direction?
• Winther: Its a very bad suggestion to return -1337. If one is not handling this case then it's better having no test for y=0 here as that will give the much more sensible results Inf or NaN when y=0.
• ferada: Welcome to Code Review! As the other comments already mention I'm also doubtful about the advice and formatting; otherwise your answer looks fine, enjoy your stay!
• Sanchises: @Winther Good luck storing NaN or Inf in an int.
• NonCreature0714: Based on feedback, I've made corrections :)