Calculator using class templates

Question

I've just completed an assignment about class templates in C++. It works fine and produces the correct output. We were given the main function and had to construct and implement the class. I'm wondering if there is a better (more efficient) way to construct and implement this simple calculator class template.

#include <iostream>
#include <string>

using std::cout;
using std::endl;
using std::string;


template <typename T1, typename T2>
class Calculator{
public:
      Calculator(T1 first, T2 second);    
      Calculator( const Calculator<T1, T2>& otherCalculator);      
      void setValue1(T1 first);
      void setValue2(T2 second);
      T1 add();
      T1 multiply();
 private:
      T1 number1;
      T2 number2;
 };

 template <typename T1, typename T2>
 Calculator<T1, T2>::Calculator(T1 first, T2 second)
      :number1(first), number2(second)
 {}

 template <typename T1, typename T2>
 Calculator<T1, T2>::Calculator(const Calculator<T1, T2>& otherCalculator)
      :number1(otherCalculator.number1), number2(otherCalculator.number2)
 {}

 template <typename T1, typename T2>
 void Calculator<T1, T2>::setValue1(T1 first){
      number1 = first;
 }

 template <typename T1, typename T2>
 void Calculator<T1, T2>::setValue2(T2 second){
      number2 = second;
 }

 template <typename T1, typename T2>
 T1 Calculator<T1, T2>::add(){
      T1 answer = number1 + number2;
      return answer;
 }

template <typename T1, typename T2>
T1 Calculator<T1, T2>::multiply(){
      T1 answer = number1 * number2;
      return answer;
}

int main(){
      Calculator<int,int> simpleCalc(42, 3);
      cout << "Adding: " << simpleCalc.add() << endl;
      cout << "Multiplying: " << simpleCalc.multiply() << endl;
      simpleCalc.setValue1( 7);
      cout << endl;

      Calculator<int,int> copyCalc( simpleCalc);
      cout << "Adding: " << simpleCalc.add() << endl;
      cout << "Multiplying: " << simpleCalc.multiply() << endl;
      cout << endl;

      Calculator<float,double> realCalc(1.41421, 2.718281828459045);
      cout << "Adding: " << realCalc.add() << endl;
      cout << "Multiplying: " << realCalc.multiply() << endl;
      cout << endl;
      return 0;
}

Answer 1

1. Don't flush where you don't need to.

   Flushing is expensive, so don't use std::endl unless you need it.
   Nearly always, the shorter '\n' suffices, which can be merged with any bordering string-literal.

   Still, that advice seems to be for your teacher, not you ;-)

2. return 0; is implicit in main if control reaches the closing brace }.
   The same is true for C since C99.

   This also seems to be one for your teacher.

3. I see you corrected your formatting since posting on SO.

   Still, 6 spaces is unusual. Either use the standard tab-stop (8), or if that's too much for your tastes, 4 is a common choice.

   Automatic formatting-tools exist which will do this and much additional formatting for you, which gain importance with the size of the project, especially the number of contributors.

4. There is no reason to write the member-functions of the template out-of-line.

   Even if you later divide your projects in multiple files, and thus have headers and implementation-files, templates are always (with really few exceptions) implemented in the header anyway, due to technical constraints.

5. There is absolutely no reason to explicitly implement the copy-constructor.
   Let the compiler provide a default one.
   Since C++11, one can also explicitly default it, preferably at the point of declaration: = default;.

   Additional benefit: Doing so re-enables move-ctor and move-assignment.

6. The 2-argument-ctor should be templated to accept any assignable types and use perfect forwarding:

   template <typename T3, typename T4, typename = decltype(T1(std::declval<T3&&>()),
       T2(std::declval<T4&&>()), void())>
   Calculator(T3&& a, T4&& b) : number1(std::forward<T3>(a)), number2(std::forward<T4>(b)) {}
   

7. The setters should accept any assignable type and use perfect forwarding for optimal performance.

   template <typename T3>
   auto setValue1(T3&& x) -> decltype(number1 = std::forward<T3>(x), void())
   { number1 = std::forward<T3>(x); }
   template <typename T3>
   auto setValue2(T3&& x) -> decltype(number2 = std::forward<T3>(x), void());
   { number2 = std::forward<T3>(x); }
   

8. add and multiply should use a deduced return-type, and be const. This transfers to avoiding explicit types in the implementations:

   • Use decltype(std::declval<T1&>()+std::declval<T2&>()) for C++11.

     decltype(std::declval<T1&>()+std::declval<T2&>()) add() const;
     decltype(std::declval<T1&>()*std::declval<T2&>()) multiply() const;
     

   • Or since C++14, just auto for letting the compiler deduce it.

     auto add() const {
         auto answer = number1 + number2;
         return answer;
     }
     

   • Or shorter:

     auto add() const {
         return number1 + number2;
     }
     

Putting it all together:

template <class T1, class T2>
class Calculator {
    T1 number1;
    T2 number2;
public:
    template <class T3, class T4, class = decltype(T1(std::declval<T3&&>()),
        T2(std::declval<T4&&>()), void())>
    Calculator(T3&& a, T4&& b) : number1(std::forward<T3>(a)), number2(std::forward<T4>(b)) {}
    // implicit: copy-ctor, move-ctor, copy-assignment, move-assignment, dtor

    template <class T3>
    auto setValue1(T3&& x) -> decltype(number1 = std::forward<T3>(x), void())
    { number1 = std::forward<T3>(x); }
    template <class T3>
    void setValue2(T3&& x) -> decltype(number2 = std::forward<T3>(x), void());
    { number2 = std::forward<T3>(x); }

    auto add() const { return number1 + number2; }
    auto multiply() const { return number1 * number2; }
};

Answer 2

In addition to the previous good answer, in order to keep your code efficient not only for the C++ primitive data types but also for the compound user defined types, I suggest:

• Make the member functions 'constructors', setValue1, setValue2, all take their arguments as const&. For instance:

  template <typename T1, typename T2>
  void Calculator<T1, T2>::setValue1(const T1 &first) {
      number1 = first;
  }