This is the original GotW problem and solution substantially as posted to Usenet. See the book Exceptional C++ (Addison-Wesley, 2000) for the most current solutions to GotW issues #1-30. The solutions in the book have been revised and expanded since their initial appearance in GotW. The book versions also incorporate corrections, new material, and conformance to the final ANSI/ISO C++ standard. |
Class Mechanics
Difficulty: 7.5 / 10How good are you at the details of writing classes? This GotW focuses not only on blatant errors, but even more so on professional style. 
ProblemYou are doing a code review. A programmer has written the following class, which shows some poor style and has some real errors. How many can you find, and how would you fix them? class Complex {
public:
Complex( double real, double imaginary = 0 )
: _real(real), _imaginary(imaginary) {};
void operator+ ( Complex other ) {
_real = _real + other._real;
_imaginary = _imaginary + other._imaginary;
}
void operator<<( ostream os ) {
os << "(" << _real << "," << _imaginary << ")";
}
Complex operator++() {
++_real;
return *this;
}
Complex operator++( int ) {
Complex temp = *this;
++_real;
return temp;
}
private:
double _real, _imaginary;
};
SolutionPrefaceThere is a lot more wrong with this class than will be shown here. The point of this puzzle was primarily to highlight class mechanics (e.g., "what is the canonical form of operator<<?", "should operator+ be a member?") rather than point out where the interface is just plain badly designed. However, I will start off with one very useful comment, #0... 0. Why write a Complex class when one already exists in the standard library? (And, incidentally, one that doesn't have any of the following problems and has been crafted based on years of practice by the best people in our industry? Humble thyself and reuse!) [Guideline] Reuse standard library algorithms instead of handcrafting your own. It's faster, easier, AND safer!
class Complex {
public:
Complex( double real, double imaginary = 0 )
: _real(real), _imaginary(imaginary) {};1. Style: This can be used as a single-parameter constructor, hence as an implicit conversion. This may not always be intended! [Guideline] Watch out for silent conversions. One good way to avoid them is to make ctors explicit when possible.
void operator+ ( Complex other ) {
_real = _real + other._real;
_imaginary = _imaginary + other._imaginary;
}2. Style: For efficiency, the parameter should be a const& and "a=a+b" should be rewritten "a+=b". [Rule] Prefer passing const& instead of copied values. [Guideline] Prefer using "a op= b" instead of "a = a op b" for arithmetic operations (where appropriate; some classes -- not any that you wrote, right? -- might not preserve the natural relationship between their op and op=).
3. Style: operator+ should not be a member function. If it's a member like this, you can write "a=b+1" but not "a=1+b". For efficiency, you may want to provide an operator+(Complex,int) and operator+(int,Complex) too.) [Rule] Prefer these guidelines for making an operator a member vs. nonmember function: (Lakos96: 143-144; 591-595; Murray93: 47-49)
- unary operators are members
- = () [] and -> must be members
- += -= /= *= (etc.) are members
- all other binary operators are nonmembers
4. ERROR: operator+ should not modify this object's value. It should return a temporary object containing the sum. Note that this return type should be "const Complex" (not just "Complex") in order to prevent usage like "a+b=c". (Actually, the original code is much closer to what operator+= should be than it is to operator+.) 5. Style: You should normally define op= if you define op. Here, you should define operator+=, since you defined operator+. In this case, the above function should be operator+= in any case (with a tweak for the proper return value, see below). void operator<<( ostream os ) {
os << "(" << _real << "," << _imaginary << ")";
}(Note: For a real operator<<, you should also do things like check the stream's current format flags to conform to common usage. Check your favourite STL book for details... recommended (if dated) are Steve Teale's "C++ IOStreams Handbook", Glass and Schuchert's "The STL <Primer>", and Plauger's "The (Draft) Standard C++ Library".) 6. ERROR: operator<< should not be a member function (see above), and the parameters should be "(ostream&, const Complex&)". Note that, as James Kanze pointed out, it's preferable not to make it a friend either! Rather, it should call a "print" public member function that does the work. 7. ERROR: The function should have return type "ostream&", and should end with "return os;" to permit chaining (so you can write "cout << a << b;"). [Rule] Always return stream references from operator<< and operator>>.
Complex operator++() {
++_real;
return *this;
}8. Style: Preincrement should return Complex& to let client code operate more intuitively. Complex operator++( int ) {
Complex temp = *this;
++_real;
return temp;
}9. Style: Postincrement should return "const Complex". This prevents things like "a++++", which doesn't do what a naive coder might think. 10. Style: It would be better to implement postincrement in terms of preincrement. [Guideline] Prefer to implement postincrement in terms of preincrement.
private:
double _real, _imaginary;
};11. Style: Try to avoid names with leading underscores. Yes, I've habitually used them, and yes, popular books like "Design Patterns" (Gamma et al) do use it... but the standard reserves some leading-underscore identifiers for the implementation and the rules are hard enough to remember (for you and for compiler writers!) that you might as well avoid this in new code. (Since I'm no longer allowed to use leading underscores as my "member variable" tag, I'll now use trailing underscores!) That's it. Here's a corrected version of the program, ignoring design and style issues not explicitly noted above: class Complex {
public:
explicit Complex( double real, double imaginary = 0 )
: real_(real), imaginary_(imaginary) {}
Complex& operator+=( const Complex& other ) {
real_ += other.real_;
imaginary_ += other.imaginary_;
return *this;
}
Complex& operator++() {
++real_;
return *this;
}
const Complex operator++( int ) {
Complex temp = *this;
++(*this);
return temp;
}
ostream& print( ostream& os ) const {
return os << "(" << real_
<< "," << imaginary_ << ")";
}
private:
double real_, imaginary_;
friend ostream&
operator<<( ostream& os, const Complex& c );
};
const Complex operator+( const Complex& lhs,
const Complex& rhs ) {
Complex ret( lhs );
ret += rhs;
return ret;
}
ostream& operator<<( ostream& os,
const Complex& c ) {
return c.print(os);
} | 
