Solution
This class has a lot of problems梕ven 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梕specially standard library code梚nstead 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梚n 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梚n 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梖or you and for compiler writers梩hat you may as well avoid leading underscores entirely. Instead, my own preference is to follow the convention of designating member variable names with a trailing underscore.
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);
}
|
