13
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ The copy constructor can be omitted or defaulted. add() and multiply() returning T1 is not a great idea but doing it correctly is probably beyond your level at the moment. As written, the functions should also be const. The interface is rather clunky - adding up two numbers requires three separate function calls, or two if you construct a fresh object. Is that required by your assignment? \$\endgroup\$ – T.C. Feb 6 '15 at 10:59
  • \$\begingroup\$ @T.C.: See "We were given the main function and had to construct and implement the class". So the answer is unfortunately yes. \$\endgroup\$ – Deduplicator Feb 6 '15 at 11:12
15
\$\begingroup\$
  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; }
};
\$\endgroup\$
  • \$\begingroup\$ You know, I'd be curious to see some profiling of when (if) all that template stuff actually pays off. (e.g. What do T1 and T2 have to be to see any speed-up? POD types? vector types? matrix types?). What proportion of that do we get if we just change the original version to use std::move when copying to the member variable? \$\endgroup\$ – user673679 Jul 9 at 8:52
10
\$\begingroup\$

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;
    }
    
\$\endgroup\$
  • 1
    \$\begingroup\$ Should have looked there too, yes. Though only also adding an overload using rvalue-references and forwarding will get the maximal performance. Or better templating this one with forwarding-references. +1 \$\endgroup\$ – Deduplicator Feb 6 '15 at 17:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.