I l@ve RuBoard

Solution

This class has a lot of problems—even more than I will explicitly show here. The point of this puzzle is primarily to highlight class mechanics (issues such as "what is the canonical form of operator<<?" and "should operator+ be a member?") rather than point out where the interface is just plain poorly designed. However, I will start off with perhaps the most useful observation: Why write a Complex class when one already exists in the standard library? And, what's more, the standard one isn't plagued with 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 code—especially standard library code—instead of handcrafting your own. It's faster, easier, and safer.

Perhaps the best way to fix the problems in the Complex code is to avoid using the class at all and use the std::complex template instead.

Having said that, let's go through the class as written and fix the problems as we go. First, the constructor:

The constructor allows an implicit conversion.
```
Complex( double real, double imaginary = 0 ) 
  : _real(real), _imaginary(imaginary)
{
}
```
Because the second parameter has a default value, this function can be used as a single-parameter constructor and, hence, as an implicit conversion from double to Complex. In this case, it's probably fine, but as we saw in Item 6, the conversion may not always be intended. In general it's a good idea to make your constructors explicit by default unless you deliberately decide to allow the implicit conversion. (See Item 39 for more about implicit conversions.)

Guideline

Watch out for hidden temporaries created by implicit conversions. One good way to avoid this is to make constructors explicit when possible, and avoid writing conversion operators.
operator+ is probably slightly inefficient.
```
void operator+ ( ( Complex other ) 
{
  _real = _real + other._real;
  _imaginary = _imaginary + other._imaginary;
}
```
For efficiency, the parameter should be passed by reference to const

Guideline.

Prefer passing objects by const& instead of passing by value.

Also, in both cases, "a=a+b" should be rewritten "a+=b". Doing that won't give you great efficiency gains in this particular case because we're only adding doubles, but it can make a significant difference with class types.

Guideline

Prefer writing "a op= b;" instead of "a = a op b;" (where op stands for any operator). It's clearer, and it's often more efficient.

The reason why operator+= is more efficient is that it operates on the left-hand object directly and returns only a reference, not a temporary object. On the other hand, operator+ must return a temporary object. To see why, consider the following canonical forms for how operator+= and operator+ should normally be implemented for some type T.
```
T& T::operator+=( const T& other ) 
{
  //...
  return *this;
}

const T operator+( const T& a, const T& b )
{
  T temp( a );
  temp += b;
  return temp;
}
```
Notice the relationship between operators + and +=. The former should be implemented in terms of the latter, both for simplicity (the code is easier to write) and for consistency (the two will do the same thing and are less likely to diverge during maintenance).

operator+ should not be a member function.

void operator+ Complex other 
{
  _real = _real + other._real;
  _imaginary = _imaginary + other._imaginary;

Guideline

If you supply a standalone version of an operator (for example, operator+), always supply an assignment version of the same operator (for example, operator+=) and prefer implementing the former in terms of the latter. Also, always preserve the natural relationship between op and op= (where op stands for any operator).

If operator+ is made a member function, as it is here, then it won't work as naturally as your users may expect when you do decide to allow implicit conversions from other types. Specifically, when adding Complex objects to numeric values, you can write "a = b + 1.0" but not "a = 1.0 + b" because a member operator+ requires a Complex (and not a const double) as its left-hand argument.

Also, if do you want your users to be able to add Complex objects to doubles conveniently, it may make sense to provide the overloaded functions operator+(const Complex&, double) and operator+(double, const Complex&) too.

Guideline

The standard requires that operators = () [] and -> must be members, and class-specific operators new, new[], delete, and delete[] must be static members. For all other functions:

if the function is operator>> or operator<< for stream I/O,

or if it needs type conversions on its leftmost argument,

or if it can be implemented using the class's public interface alone,

make it a nonmember (and friend if needed in the first two cases)

if it needs to behave virtually,

add a virtual member function to provide the virtual behavior,

and implement it in terms of that

else

make it a member

operator+ should not modify this object's value, and it should return a temporary object containing the sum.
```
void operator+ ( Complex other ) 
{
  _real = _real + other._real;
  _imaginary= _imaginary + other._imaginary;
}
```
Note that the return type for the temporary should be "const Complex" (not just "Complex") in order to prevent usage like "a+b=c".
You should normally define op= if you define op. Here, you should define operator+=, since you defined operator+, and then implement the latter in terms of the former.
operator<< should not be a member function.
```
void operator<< ( ostream os ) 
{
  os << "(" << _real << "," << _imaginary << ")";
}
```
See again the similar discussion above about operator+. Also, the parameters should be "(ostream&, const Complex&)". Note that the nonmember operator<< should normally be implemented in terms of a(n often virtual) member function that does the work, usually named something like Print().

Also, for a real operator<<, you should do things like check the stream's current format flags to conform to common usage. Check your favorite standard library or iostreams book for details.
Further, operator<< should have a return type of "ostream&" and should return a reference to the stream in order to permit chaining. That way, users can use your operator<< naturally in code like "cout << a << b;".

Guideline

Always return stream references from operator<< and operator>>.
The preincrement operator's return type is incorrect.
```
Complex operator++() 
{
  ++_real;
  return *this;
}
```
Preincrement should return a reference to non-const—in this case, Complex&. This lets client code operate more intuitively and avoids needless inefficiency.
The postincrement operator's return type is incorrect.
```
Complex operator++( int ) 
{
  Complex temp = *this;
  ++_real;
  return temp;
}
```
Postincrement should return a const value—in this case, const Complex. By not allowing changes to the returned object, we prevent questionable code like "a++++", which doesn't do what a naïve user might think it does.
Postincrement should be implemented in terms of preincrement. See Item 6 for the canonical form for postincrement.

Guideline

For consistency, always implement postincrement in terms of preincrement. Otherwise, your users will get surprising (and often unpleasant) results.
Avoid reserved names.
```
private: 
  double _real, _imaginary;
```

Yes, popular books like Design Patterns (Gamma95) do use leading underscores in variable names, but don't do it. 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 may as well avoid leading underscores entirely^[1]. Instead, my own preference is to follow the convention of designating member variable names with a trailing underscore.

^[1] For example, names with leading underscores are technically reserved only for nonmember names, so some would argue that this isn't a problem and that class member names with leading underscores are fine. That's not entirely true in practice, because some implementations #define macros with leading-underscore names, and macros don't respect scope. They'll tromp on your member names as easily as they'll tromp on your nonmember names. It's easier just to leave all leading underscores for implementers and avoid the possible hassles.

That's it. Here's a corrected version of the class, 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_;
  };

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

I l@ve RuBoard