c++ classes , this works but can someone make it better so it meets standards/coding practices

#include <iostream>

using namespace std; 




class Calculator
{
        public: 
          int number1; 
          int number2 ; 

        public: 

        void  setCalcNumbers(int input1 , int input2 )
          {

          number1 = input1; 
          number2 = input2; 
          }
          int addNumber()
          {
              return number1 + number2;
          }
          int subtractNumber()
          {
              return number1 - number2;
          }
          int divideNumber()
          {
              return number1 / number2;
          }
          int multiplyNumber()
          {
              return number1 * number2;
          }

};

int main()
{
    int numberInput1 = 0; 
    cout << "Enter number 1: "; 
    cin >> numberInput1; 


    int numberInput2 = 0;
    cout << "Enter number 2: "; 
    cin >> numberInput2;

    Calculator t; 
    t.setCalcNumbers(numberInput1, numberInput2);

    char userOperationChoice; 
    cout << "which operation would you like to perform? "
         << " , enter M for Multiplication, D for Division, A for addition or S for Subtraction:" << endl;

    cin >> userOperationChoice; 
    char a,d,m,s;
    switch (userOperationChoice) 
    {

    case 'a'  : 
         t.addNumber();
         cout << "the total is: " << t.addNumber() << endl; 
         break; 

    case 's':
         t.subtractNumber(); 
         cout << "the total is: " << t.subtractNumber() << endl; 
         break ; 
    case 'd': 
         t.divideN开发者_运维百科umber();
         cout << "the total is: " << t.divideNumber() << endl; 
         break; 
    case 'm':
         t.multiplyNumber();
         cout << "the total is: " << t.multiplyNumber() << endl; 
         break; 

    }        



    system("pause"); 
    return 0 ; 
}

---

Not sure if this is homework, so I will give the extra short version. Particularly, I would go down the function template path.

template <typename T> T  add(T n1,T n2){                                        
        return n1 + n2; 
}

and use it:

 int s = add<int> (1,3); 

Considering that you ask about classes , I would eliminate the setNumbers function, and make every function static, and receiving as parameters the numbers to operate on. Additionaly, I would make it a template class.

Why types are important ? Well, right now your calculator can only operate on integer numbers. So what happens if the user inputs floating point numbers, such as 1.5 and 3.2 ? As you are using int types, the chosen operation will be between the numbers 1 and 3, as they are truncated. Not so bad? Well, consider this case. The user inputs numbers 3 and 0.5, and chooses to divide. What will happen ? Yes, the black hole generator: division by zero. So, the template class / function is the appropiate way. If you are only looking for an integer calculator, I'd suggest renaming the class to integerCalculator and doing some validation on the user input (for avoiding fatal errors such as dividing by zero more gracefully).

For example

The template class

 template <typename T> calculator {
      static T add (T n1 ,T n2){
        return n1 + n2;
      }
    };

The integer calculator

class integerCalculator{
  public:
          //other functions

          int divideNumber(){
                 if (number2 == 0) //handle error
                 return number1/number2;
          }
};

Some more on your version:

number1 and number2 should not be public, or if the are, why do you need a setNumber() function? Reduce their visibility (private seems apropiate).

class Calculator{
        int n1;
        int n2;

   public:  void setNumbers(int n1,int n2){...}
   //add, divide, multiply, subtract functions
}

Seems that the a,d,m,s variables are unused. If they are not part of the solution...

---

1. define your variables top of Main function;
2. try to setting " default " section for " switch " statement;

have fun ...

---

char a,d,m,s;

before the Switch statement ? Why do you have them/need them there ?

---

1. make all those functions const i.e. int multiplyNumber () const;
2. You could consider operator overloading because that's a bit more intuitive in many situations.
3. It's probably a typo, but you should only have 1 public: section. The data members should be private.

4. Create a constructor with initializers:

   Calculator (const int n1, const int n2) : number1(n1), number2 (n2) { }

5. Besides function templates as in another answer already, standard practice would be to make this a class template to accept any data type, not just int.

   template <typename T> class Calculator { /* ... */ }

Thus you could declare number1 and number2 as type T.

---

I am going to be a bit rash, but please understand it's for your good ;)

The class Calculator is useless. It serves no purpose here. A class is meant to encapsulate a state and control the access to this state so that it can maintain a number of invariants.

For example, if you think of a dynamic array with the following members:

size_t mNumberItems; // number of items in the array
size_t mCapacity;    // maximum number of items in the array
T* mElements;        // array

The following invariants hold:

• mNumberItems <= mCapacity
• mNumberItems represents the number of items in the array, it's always in sync
• mCapacity reprents the maximum capacity of the array, it's also in sync

So I would consider rewriting the code as:

int getNumber(char const* text)
{ 
  int number = 0;
  std::cout << text << std::endl;
  std::cin >> number;
  return number;
}

int main(int argc, char* argv[])
{
  int n1 = getNumber("Please enter the first number:");
  int n2 = getNumber("Please enter the second number:");

  char userOperationChoice; 
  cout << "which operation would you like to perform?\n"
       << "Enter * for Multiplication, / for Division, "
       << "+ for addition or - for Subtraction:" << endl;

  cin >> userOperationChoice;

  int total = 0;
  switch(userOperationChoice)
  {
  case '*': total = n1 * n2; break;
  case '/': total = n1 / n2; break;
  case '+': total = n1 + n2; break;
  case '-': total = n1 - n2; break;
  default:
    std::cout << userOperationChoice
              << " is not in the set of operators allowed" << std::endl;
    return 1;
  }

  std::cout << "Total is: " << total << std::endl;
}

It's not perfect (lack of input validation on the numbers), but I have factorized much.