GotW #4: Class Mechanics (7/10)

How good are you at the details of writing classes? This item focuses not only on blatant errors, but even more so on professional style. Understanding these principles will help you to design classes that are easier to use and easier to maintain.

 

Problem

JG Question

1. What makes interfaces “easy to use correctly, hard to use incorrectly”? Explain.

Guru Question

2. You 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 r, double i = 0 )
: real(r), imag(i)
{ }

void operator+ ( complex other ) {
real = real + other.real;
imag = imag + other.imag;
}

void operator<<( ostream os ) {
os << "(" << real << "," << imag << ")";
}

complex operator++() {
++real;
return *this;
}

complex operator++( int ) {
auto temp = *this;
++real;
return temp;
}

// ... more functions that complement the above ...

private:
double real, imag;
};

Note: This is not intended to be a complete class. For example, if you provide operator++ you would normally also provide operator–. Rather, this is an instructive example to focus on the mechanics of writing correctly the kinds of functions this class is trying to support.

34 thoughts on “GotW #4: Class Mechanics (7/10)

  1. I’m going to address one tiny part of this – the streaming. I’ve always hated this form of implementing printing. I don’t mean the fact that it is a member vs non-member function. I mean the assumption that one form of printing is good or correct. Perhaps it is due to the fact that I do a lot of map applications, but why the assumption that I have only one way to ‘print’ (stream) something? In my application domain, a latitude can be printed as decimal degrees (32.3443534N), as deg/min, as deg/min/sec, we can use N/S or +/- to denote hemispheres, and so on. I may want to stream it to a file in a different format, to a SQL database in yet another format, and so on.

    Also, you may need to stream to the cout stream, you may need to use Windows’ WriteFile function, or who knows what sink.

    So, first of all, any kind of printing or streaming does not belong in the class. And, it should not assume some single representation. Hey, if your app requires a single representation,sure, go for it, but I would think that a class like Complex would span across more than one app if you are working in a space where you need complex numbers.

    And, of course, this considers only half of the issue. Why are we implementing streaming to output, but not streaming from input to the class? Perhaps that makes the wide variety of possible formats clearer – you may be reading a file generated from a 3rd party app, you might be reading user input with errors in it, and so on. Clearly there is no ‘one size fits all’ solution here. Yet we so often treat output that way.

    In short, I almost never use the <> operators to define streaming for a class because it is something that reasonably has to vary (to be fair, I’ve always worked in industries where a piece of code like this is likely to hang around and be used for at least a few decades – I’d write a stream function like this in a second for ‘throwaway’ code)

  2. Good interface design is an exercise in tradeoffs. There is no single meaning of something like “easy to use correctly, hard to use incorrectly” – like everything in life, it depends on context. This is a list of things to consider. It is a rare situation indeed where there is any way to do all of these things at the same time. It’s a trade-off. In many cases, some of these items can’t be done no matter what you do.

    * If the main concern for your program is performance, and the interface will participate in performance-critical computations, then avoid designing an interface that makes it difficult or impossible to achieve top performance. In this case, correct use of an interface implies efficient use. For example, avoid unnecessary copying and memory allocation when possible.

    * Make method names succinctly, aptly and clearly describe what the method does. I know what obj.setX(5) probably does, but I’m much less sure about obj.adjustX(5). Use long method names if that is necessary.

    * Follow the conventions of the code that the interface is a part of.

    * Avoid abbreviations unless you are sure that the user of the interface will understand them easily. Abbreviating pointer to ptr is probably fine. Abbreviating “the identity function” to “id” is only OK if you know for a fact that everyone who will have to deal with your interface is going to make the connection easily.

    * If clearly describing what a method does requires a 30+ character name, try to see if you can’t change what the function does so that you can give it a shorter name. Perhaps split it into two simpler methods.

    * If a method is unavoidably so complicated that a user of it simply must read the documentation, give it a name that will trigger people to go look at the documentation. Don’t let obj.setX(5) do something complicated – I’m going to think that I know what it does and I’ll be wrong. Rename it to something complicated or mysterious (yet still apt) so people will go look up its documentation because they realize that they can’t figure out what that means without looking at the documentation. First, try to redesign the interface so you don’t need that method in the first place.

    * Consider how your interface will show up in intellisense and compiler error messages. For example, if you have a public foo(int) and a private foo(MyInternalType), users of your interface are going to see the private foo in error messages and in intellisense, which probably isn’t what you want. Give the private foo a different name (oh, and don’t name your methods foo).

    * Avoid methods with many parameters of the same type. If you’ve got a function like foo(bool, bool, bool, bool), then likely each parameter does something different and it will be difficult for people to see at a glance whether the order of the parameters is correct. Sometimes there’s no good way around this, but a possible replacement would be fooA(bool), fooB(bool), fooC(bool) and fooD(bool), where A, B, C and D are descriptions of what those bools do.

    * Avoid designing an interface where obj.doSomething(false) causes something to be done. For example, doSomething(bool) might do what it does in a different way depending on the parameter. This is confusing. It looks like you are asking for something not to be done.

    * Suppose that you know that most of the people who will be using the interface do not understand some advanced concept, like template-templates in C++ or topologically sorted directed acyclic graphs. If you must use such a concept anyway, you need to document the concept carefully, perhaps by offering a link to an online resource. First, try to avoid using the concept.

    * Minimize preconditions. For example, suppose you take a std::vector& as a parameter and that you need this vector to be sorted and that you need to hold on to the vector for a while. You could have a precondition stating that the vector must be already sorted and that it must stay alive and be unchanged for the life-time of your object. Instead, you could simplify the precondition by copying the vector and then sorting it yourself.

    * Avoid potentially bad interactions between the methods in the interface. If every call to foo() must be followed by a call to bar(), and if something bad happens if you do it the other way around, then that makes the interface harder to use correctly. There’s often no good way around this.

    * Design the interface so that it is possible to follow the Liskov substitution principle. From Wikipedia: “if S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., objects of type S may be substituted for objects of type T) without altering any of the desirable properties of that program (correctness, task performed, etc.).”

    * Minimize visible state in your interface. The dynamic type of an object is one example of a state. That kind of state is accessed through virtual functions and the Liskov substitution principle is essentially stating that the client code should not have to know the sub-type. More generally, it is desirable for an interface not to have any state at all, or if it does have state, then client code should ideally not have to worry about what the state is. Some kinds of state are not a problem, for example because it is very simple (setName() and getName()) or because it’s not visible (a transparent cache).

    * If your interface participates in parallel computation, make it very clear what the contract for parallel use of the interface is.

    * Design the interface so that it is possible to use the type system to check for bugs. You do that by making as many bugs as possible cause compile-time errors or at least warnings. You could try to expand this by making your interface easy for static analysis tools to understand, perhaps by annotating it.

    * Design your interface so that there is no tricky or repetitive thing that the user of the interface has to remember to do. Returning a pointer that has to be deleted is a great/horrible example of that (use a smart pointer). Returning an error code that must be checked is another example (use exceptions).

    * Offer exception guarantees. Offer the strong exception guarantee when you can. If you can’t, offer the weak one. Document what guarantee you are giving – maybe not always, but at least in all cases where the answer is not what one would expect.

    * Don’t design an interface that can’t be implemented without using global state. (Singletons are global state).

    * Design the interface so that it is possible for the implementation of the interface to check that it is being used correctly at run-time. For example, if the parameter t to obj.foo(t) must be less than some number X, then you could make it mandatory to inform obj about X, so that t can be checked inside foo, even if it would not otherwise be necessary to know the value of X to implement the interface.

    * Make it testable. If you can’t test to the interface in a reasonable way, there’s more likely to be bugs that you won’t be aware of until the worst possible time. It’s hard to use an interface correctly if it wasn’t implemented correctly.

    * Encapsulate. The interface should not reveal how it is being implemented.

    * Don’t do too much. An interface with too many responsibilities tends to be hard to use.

    * Make it general. You want to be able to use this interface in different contexts, so don’t make your interface specific to a context for no reason.

    * Do something well. If you make your interface too general, you might end up with something that will work in every relevant circumstance, but that doesn’t fit well to any specific use. Make sure your interface is a good fit in at least one situation.

    * Consider the impact of your interface on the whole program’s complexity – both statically and at run-time. It is sometimes possible to make an interface really simple and nice-looking, but programs that use that interface end up having to build very complicated structures in memory. For example, the observer pattern is simple (and sometimes a good choice), yet you can use it to create an impossibly complex maze of calls-backs at run-time. If your interface encourages or even requires that sort of thing, then that makes it harder to use correctly. Sometimes it’s better to complicate your interface if the result is that client code ends up constructing simpler structures in memory. Pushing complexity from your interface into the surrounding code or into the run-time characteristics of the program isn’t always a good idea.

  3. To those suggesting that the constructor be made explicit, it’s worth noting out that std::complex has a constructor of the form (note the lack of an “explicit” keyword):

    constexpr complex(double re = 0.0, double im = 0.0);

    As litb1 pointed out, making the constructor explicit will prevent the use of a C++11 initializer list for copy-initialization. For example, the following won’t compile:

    void f(complex c);
    f({ 1.0, 1.0 });
    f({ 1.0 })

    I think in this case an implicit conversion is natural, and I don’t see it leading to unintended side-effects (unlike, say, going from std::string to const char *).

  4. @Crazy Eddie
    I disagree with your idea of adding getters to this class.

    A complex number can be defined in different ways, one of them being the sum of a and ib, as it is here. Another implementation could rely on its absolute value and its argument.

    To make an example, consider a complex class implemented in term of absolute value and argument:

    class complex
    {
    public:
        double getReal() const { return /* formula */; }  
        double getImaginary() const { return /* formula */; }
    
    private:
        double argument;
        double absoluteValue;
    };
    

    Now what would operator==() code be?

    bool operator==(const complex& a, const complex& b)
    {
        return (a.getReal() == b.getReal()) && (a.getImaginary() == b.getImaginary());
    }
    

    This probably won’t work because of round-up errors, and even if it does, it will under-perform because of all the computation needed by getReal() and getImaginary(). This is why operator== needs access to the inner guts of the class, in a perfect world it would really be part of the class, but for design reasons (namely, symmetry), it was cast away.

  5. You also need to consider multi element initializer lists when making a constructor “explicit” (see the tuple “explicit” constructor).

    The question becomes, do you want this to work?

    void f (complex c);
    f ({ 0.f, 1.f });

    Simply making the two parameter constructor explicit just to guard against the one-argument case then is unsuitable because it guards against the multi arg case too. You will need to overload it (possibly using constructor delegation in the implementation).

  6. “Just focusing one issue I haven’t seen mentioned by others already, I’d question the appropriateness of offering the increment operators at all.”

    I had a similar thought when I read the class, “what exactly does it mean to increment a complex number?”. I don’t think I’ve ever used increment on a floating point number.

  7. To make an easy to use interface:

    1. Make it impossible or very hard to use incorrectly (fail to compile, crashes, etc when used incorrectly – see std::chrono as a simple example) – this is the most important guideline for type design because it both prevent errors by mistake and, more importantly, it drives the usage (it’s like walls for a blind: when you touch it you know you can’t go this way);
    2. Make sure each operation have only one way to be done – this actually improve composition (note that if two functions does the same thing but in a different way that impact the semantic, it’s two operations);
    3. Hide complexity. Unfortunately it is not always efficient to hide totally the implementation details of our types (maybe it will be with modules but don’t count on it until we got implementations), but if you can hide the details, the only thing visible is how to use it.
    4. Give a unique responsability to the type, not more.
    5. Make it as short as possible, that is, a minimum of functions (depends on the domain and responsability);
    6. Use names as obvious as possible in the domain context;
    7. Document the interface. I put this at last because I consider it of last resort but you always end up doing it for member functions doing not that obvious operations.

  8. Interfaces are easy to use correctly if they follow established conventions and best practices. (Ideally your established conventions will reflect best practice, but best practice evolves, so the longer your conventions have been established the greater the danger they are out of date.) They should follow the principle of least surprise. Ideally a reasonably skilled programmer using them would not need to look up the documentation to see how they work.

    The example class is presumably intended to be a numeric type, so it should follow the conventions established by int, double and other numeric types. It doesn’t. Note that its definition of operator+() isn’t [i]intrinsically[/i] wrong; it isn’t impossible to use correctly: but it is different to how operator+() works for double so very liable to be used incorrectly. It doesn’t matter if the documentation describes the idiosyncratic behaviour correctly and lucidly. Few programmers will think to read it.

    Programmers have ample opportunities to be creative. Deciding the behaviour of operator+() is not one of them.

  9. Another thought: if we do not make it a template (which we possibly do not want) the whole class is actually better off being a constexpr class. It won’t hurt, it’ll help in some cases and it is certainly a good thing to do with classes this simple.
    It won’t optimize anything, but lets you actually say in your code that that whole bunch of complex arithmetics on constants should run at compile-time.

  10. Good points in the previous comments.

    One more thing which comes to mind — given that it makes sense for “complex” to have value semantics (as opposed to reference semantics), we’d probably want to consider preventing inheritance (for the same reason it’s not a good idea to inherit from an STL container).

    As in:
    Guideline #4: A base class destructor should be either public and virtual, or protected and nonvirtual.
    http://www.gotw.ca/publications/mill18.htm

    I don’t see the case for public & virtual (since I don’t see the case for our “complex” class having reference semantics). Hence, I’d go with the other alternative.

    This is where C++11 comes in, as it allows us to use “final” to prevent inheritance:
    http://en.cppreference.com/w/cpp/language/final

  11. Just focusing one issue I haven’t seen mentioned by others already, I’d question the appropriateness of offering the increment operators at all.

    The increment and decrement operators make intuitive sense for discrete types, like counters, chars, certain enumerations, even bools.

    Perhaps because I’ve always thought of these operators as analogues of Pascal’s succ() and pred(), it’s always seemed odd to me that C and C++ provide them for floating point types. What does it mean to increment a type that approximates a continuous quantity? It’s not intuitive (to me), given a double d, what

    ++d

    would mean. Does it increment by 1.0 or does it increment by some epsilon to get to the next higher, representable value? Both would be useful, and, given that I could always write

    d += 1.0;

    for the former, defining operator++ for the latter would make the most sense. But alas, that’s not the case. That might just be my own bias.

    Even if I make peace with increment and decrement on a double, what should they mean on a complex? That’s even less intuitive. A complex is a two-dimensional quantity. Should increment affect the real part or the imaginary part or both? It’s simply not obvious–at least not to everyone.

    In the interest of making it easy to use correctly and difficult to use incorrectly, I’d leave out the increment and decrement operators for this class at least until a client of the class made a strong case for them. (A strong case might be that having these operators enables a class or function template that otherwise can’t use them.) In the mean time, a client can write

    c += 1.0;

    , which is not only correct, it’s _obviously_ correct. To everyone.

    To me, that’s the difference between a well-implemented class interface and a well-_designed_ one.

  12. 1. What makes interfaces “easy to use correctly, hard to use incorrectly”?

    The first answer coming to my mind is “Documentation”.
    The standard classes are quite well documented – imagine how difficult it would be to use them correctly without this documentation, and how easy it would be to use them incorrectly.

    It makes me a little sad that no one mentioned this so far.

    Among the very effective programmers that I got to know, the share of those who actually write good documentation is surprisingly low. There are even some who explicitly argue against documentation. I often wonder why this is so.

    Reasons might be:

    * Documentation is (usually) not being paid for. What usually matters is functionality.

    * Not every one is a born writer. Some very effective programmers that I got to know write surprisingly bad natural language. My guess is that most of these actually know this (but some don’t :-). Many people dislike doing things that they are not good at. Thus I assume that some simply hate writing documentation because it makes them feel bad, or even inadequate.

    * Writing good documentation is a skill that by most people must be learned. This takes at least effort and time, and thereby usually money.

    * Tools that check for language errors in documentation are not widespread. This is in contrast to the code, where the compiler and tools like lint can help to work out the kinks.

    Granted, in contexts where the code is not likely to ever be read, writing documentation has only one benefit that I can think of: The writer can hone his writing skills.

    However, in contexts where the code is likely to be read, maintained or reused, documentation is crucial. In my professional experience (15y), this is the case in by far the majority of projects. In some circumstances, especially when key people leave a project, missing or bad documentation can become extremely expensive.

  13. Just had to give it a final try.

    class complex 
    {
    	// allow access to private members
    	friend ostream& operator<<( ostream& os, const complex& c );
    
    public:
    	// doubles as default ctor
    	complex( double r = 0, double i = 0 ) : real(r), imag(i) { }
    
    	// to efficiently implement operator+ 
    	complex& operator+=(const complex& c) 
    	{
    		real += c.real;
    		imag += c.imag;
    
    		return *this;
    	}
    
    	// return reference to result
    	complex& operator++()
    	{        
    		++real;        
    		return *this;    
    	}    
    
    	// return unchanged object by value
    	complex operator++( int ) 
    	{        
    		auto temp = *this;        
    		++real;        
    		return temp;    
    	}   
    
    	// ... more functions that complement the above ...
    private:    
    	double real, imag;
    };
    
    // IO operator cannot be a class member. 
    // Class member ops require the left operand to be a class object
    ostream& operator<<( ostream& os, const complex& c ) 
    {        
    	os << "(" << c.real << "," << c.imag << ")";
    	return os;
    }
    
    // allows implicit conversion of operands
    complex operator+ ( const complex &c1, const complex& c2 ) 
    {   
    	complex c(c1);
    	c += c2; 
    	return c;
    }  
    
  14. Think I would as reviewer simply say use a std:: complex or other library and go off and have lunch!
    But was never any good with made up exam questions….

    ps Unfortunately saw similar when someone was writing a linked list class. It was a good lunch!

  15. Sorry, forgot default ctor.

    class complex{
       complex() = default;
       complex( const complex& other) = default;
       complex& operator = (const complex& other) = default;
       // other method and ctors   ..............................
    }
    
  16. 
    class complex 
    {
    public:
       constexpr explicit complex( double r, double i = 0.0 ) noexcept
            : real_ { r }
           , imag_ { i }
        { }
    
       constexpr complex operator+ ( const complex& other ) const noexcept
       {
            return complex( real_ + other.real_,  imag_ + other.imag_ );
        }
    
      complex& operator++() noexcept
       {
            ++real_;
            return *this;
        }
    
        complex operator++( int )  noexcept
       {
            auto temp = *this;
            ++real_;
            return temp;
        }
    
        // ... more functions that complement the above ...
        
        constexpr double real()const noexcept { return real_; }
        constexpr double imaginary() const noexcept { return imag_;}
        
    private:
        double real_ = 0.0;
        double imag_ = 0.0;
    };
    
    std::ostream&  operator << (std::ostream& os,  const complex& c)
    {
           return os << '(' << c.real() << ',' << c.imaginary() << ')' ;
    }
    
    
  17. Thought I’d give it a try

    using namespace std;
    
    // IO operator cannot be a class member. 
    // Class member ops require the left operand to be a class object
    ostream& operator<<( ostream& os, const complex& c ) 
    {        
    	os << "(" << c.real << "," << c.imag << ")";
    
    	return os;
    }
    
    complex operator+ ( const& complex c1, const& complex c2 ) 
    {   
    	complex c = c1;
    	c.real += c2.real; // '+=' assumed
    	c.imag += c2.imag;
    
    	return c;
    }  
    
    class complex 
    {
    	// allow access to private members
    	friend ostream& operator<<( ostream& os, const complex& c );
    	friend complex operator+ ( const& complex c1, const& complex c2 );
    
    public:
    	complex( double r = 0, double i = 0 ) : real(r), imag(i) { } // doubles as default ctor
    
    	complex& operator++() // return reference to result
    	{        
    		++real;        
    		return *this;    
    	}    
    
    	complex operator++( int ) 
    	{        
    		auto temp = *this;        
    		++real;        
    		return temp;    
    	}   
    
    	// ... more functions that complement the above ...
    private:    
    	double real, imag;
    };
    
  18. I figured using user literals would helps with syntax and avoid implicit cast problems from float(i.e. was the float a real or imaginary). Ended up with a lot of code though.


    #include

    using namespace std;

    //template
    class Imaginary
    {
    public:
    constexpr Imaginary() = default;
    constexpr Imaginary(const Imaginary&) = default;
    Imaginary& operator=(const Imaginary&) = default;
    explicit constexpr Imaginary(double _value) : m_Value(_value) { }

    Imaginary& operator-()
    {
    m_Value = -m_Value;
    return *this;
    }

    Imaginary& operator+=(const Imaginary& _other)
    {
    m_Value += _other.m_Value;
    return *this;
    }

    ostream& PrintToStream(ostream& _stream) const
    {
    _stream << m_Value << 'i';
    return _stream;
    }

    private:
    //Don't want getters/setters for this, don't want a way to treat this as a standard float.
    double m_Value = 0;
    };

    Imaginary operator+(const Imaginary& _arg1, const Imaginary& _arg2)
    {
    Imaginary result = _arg1;
    result += _arg2;
    return result;
    }

    ostream& operator<<(ostream& _stream, const Imaginary& _value)
    {
    return _value.PrintToStream(_stream);
    }

    //could use _fi or something for Imaginary and then templatize the Complex and Imaginary class
    constexpr Imaginary operator"" _i (long double _value)
    {
    return Imaginary(static_cast(_value));
    }

    constexpr Imaginary operator"" _i (unsigned long long _value)
    {
    return Imaginary(static_cast(_value));
    }

    class Complex
    {
    public:
    constexpr Complex() = default;
    constexpr Complex(const Complex&) = default;
    Complex& operator=(const Complex&) = default;

    constexpr Complex(double _real, const Imaginary& _imag) : m_Real(_real), m_Imag(_imag) {}
    constexpr Complex(double _real) : Complex(_real, 0_i) {}
    constexpr Complex(const Imaginary& _imag) : Complex(0, _imag) {}

    Complex& operator+=(const Complex& _other)
    {
    m_Real += _other.m_Real;
    m_Imag += _other.m_Imag;
    return *this;
    }

    Complex& operator++()
    {
    *this += 1;
    return *this;
    }

    Complex operator++(int)
    {
    Complex result = *this;
    ++(*this);
    return result;
    }

    ostream& PrintToStream(ostream& _stream) const
    {
    _stream << m_Real << "+" << m_Imag;
    return _stream;
    }

    private:
    double m_Real = 0;
    Imaginary m_Imag = 0_i;
    };

    Complex operator+(const Complex& _arg1, const Complex& _arg2)
    {
    Complex result = _arg1;
    result += _arg2;
    return result;
    }

    Complex operator+(double _real, const Imaginary& _imag)
    {
    return Complex(_real, _imag);
    }

    Complex operator+(const Imaginary& _imag, double _real)
    {
    return Complex(_real, _imag);
    }

    ostream& operator<<(ostream& _stream, const Complex& _value)
    {
    return _value.PrintToStream(_stream);
    }

    int main(int argc, const char * argv[])
    {
    auto im = -4_i;
    cout << im << endl;

    auto im2 = 2_i + im;
    cout << im2 << endl;

    Complex cx1 = 1 + 3_i;
    cout << cx1 << endl;

    Complex cx2 = 4_i + -3;
    cx2 += cx1;
    cout << cx2 << endl;

    cout << ++cx2 << endl;
    cout << cx2++ << endl;
    cout << cx2 << endl;

    cx2 = cx2 + cx1;
    cout << cx2 << endl;

    cx2 += -14_i;
    cout << cx2 << endl;

    return 0;
    }

  19. The notion of “easy to use correctly and hard to use incorrectly” is generally a good idea, but not always. As an example, a simple 2D mutable Point class is easiest to use correctly when it just has two public members and a constructor or two. Improbable as it may seem, for double>/code> members Visual C++ has a hard time optimizing optimizing code that accesses the members indirectly, or at least, it still was a bit deficient re such optimizations the year before last, so this has also to do with fundamental low-level efficiency.

    The "easy to use correctly" has to do with the class as a provider of practically useful operations and with the class as a single well understood abstraction, which is more at the language independent design level, while the "hard to use incorrectly" has to do with the class as an enforcer of design constraints, which is more at the language specific and even compiler specific level, Accommodating Visual C++'s optimization problems is an example of possibly valid compiler-specific concerns. I think such choices need to be DOCUMENTED in order to be understood by maintenance programmers or even just by client code programmers.

    Having noted that efficiency is a valid consideration, one should also note that it's easy to walk into the premature optimization trap. And the class presented in this question looks like an example of that. Apparently it's meant to provide a convenient increment operator, a operator++, but that would be better provided for a class derived from std::complex&lt>T (derived class in order to minimize the problem of multiple definitions of such an operator).

    The above addresses some main concerns of the "JG" question, question 1. Others have already discussed the trivia of the "Guru" question, question 2, what the compiler can tell you if you give it the code. I think the "Guru" designation for that is a bit misplaced: nowadays it's not hard to feed some code to a compiler.

    So far the existing answers have failed to mention what a compiler can't tell you, such as the pros and cons of choosing `void` as result type for `operator++`. Much in the same vein as choosing an as yet somewhat unconventional signed size type. This has to do with the more challenging "hard to use incorrectly" aspect – challenging in part because some established conventions in C++ are at odds with that goal.

  20. Well, others have mentioned that the postfix ++ should be based on prefix ++ so I wont reiterate that. The problem with << being part of the class likewise has been covered, but making it a friend is a mistake.

    What this class needs is 'getters' for real and imag. Then + and << can be implemented as non-friend functions. Although setters for simple data in a class is sometimes a mistake, it's warranted here. The sanity that the operators provide is simply to keep two values locked together in a sensible way. There are a great many reasons why we'd want getters for real and imag and the class is rather useless without them.

    Adding the getters (not necessarily setters) also obeys the principle that you should be able to tell if two objects are equal based on their public interface alone. In other words, we want to be able to write == as a non-friend function as well. I forget where I got this principle but it's a good one.

    That said, operator + should defer to +=, which should be added.

    complex operator + (complex left, complex const& right)
    {
      return left += right;
    }
    

    The compiler will get rid of the unnecessary temporaries either by using move construction or RVO.

  21. Follow-up:

    My bad. It is generally better to write operator+ as a friend for classes with implicit constructors:

    //... inside the class
    friend complex operator+(const complex& lhv, const complex& rhv) {
        return { lhv.real+rhv.real, lhv.imag+rhv.imag };
    }
    //...
    

    This way both cmplx + 2.2 and 2.2 + cmplx will work.

  22. // 1. why not use std::complex?
    class complex {
    public:
        // 2. where's the default/move/copy constructor?
        complex() = default;
        complex(const complex&) = default;
        // the 0 instead of 0.0 is not really an issue, but why not be a kind Samaritan?
        complex( double r, double i = 0.0 ): real(r), imag(i) { }
    
        // 3. this was messed up
        complex operator+ (const complex& other ) {
            return { real+other.real, imag+other.imag };
        }
    
        // 4. this was way totally messed up
        friend ostream& operator << (ostream& os, const complex& cm) {
            return os << "(" << cm.real << "," << cm.imag << ")";
        }
    
        // 5. as was this
        const complex& operator++() {
            ++real;
            return *this;
        }
    
        // 6. the postfix ++ was actually the only operator that worked as it should
        // but it's generally better to write it using prefix ++
        complex operator++( int ) {
            auto temp = *this;
            ++*this;
            return temp;
        }
    
        // ... more functions that complement the above ...
    
    private:
        double real, imag;
    };
    
    
  23. Joe “I don’t think explicit is necessary because implicit conversion isn’t possible with multi-parameter constructors.”

    It is when all but one of the parameters is defaulted.

  24. template<typename FloatType = double>
    class complex {
    public:
        inline complex(const FloatType r = FloatType(),const FloatType i = FloatType() )
            : real{r}, imag{i}
        { }
     
        complex& operator+= (const FloatType& r) {
            real += r; 
            return *this;
        }
     
        friend
        inline complex operator+ (const complex& a, const complex& b) {
            return {a.real + b.real, a.imag + b.imag};
        }
     
        friend
        inline ostream& operator<<( ostream& os, const complex& cp ) const {
            return os << "(" << cp.real << "," << cp.imag << ")";
        }
     
        inline complex& operator++() {
            ++real;
            return *this;
        }
     
        inline complex operator++( FloatType ) {
            auto temp = *this;
            ++real;
            return temp;
        }
     
        // ... more functions that complement the above ...
     
    private:
        FloatType real, imag;
    };
    
  25. 0. implicit conversion constructor could be undesired
    1. missing includes iosfwd
    2. ostream not by reference
    3. operator << wrong param ordering
    4. operator << "wrong" return type (no chaining)
    fixing operator<<:

        // in class
        friend std::ostream& operator<<( std::ostream& os, complex const& c); 
    
        // in cpp
        #include <ostream>
    
        std::ostream& operator<<( std::ostream& os, complex const& c) {
            return os << '(' << c.real << ',' << c.imag << ')';
        }
    

    5. operator+() actually implements more of `operator+=` and could take a const& to allow for rvalues (implicit conversions)

    fixing it as operator+ (note const):

        complex operator+ ( complex const &other ) const {
            return { real + other.real, imag + other.imag };
        }
    

    6. operator++() (prefix version) should return by reference

    7. Consider using friend operator+ instead of member operator+ to allow for implicit conversion of the first operand as well:

        friend complex operator+ ( complex const& a, complex const &other )  {
            return { a.real + other.real, a.imag + other.imag };
        }
    

    Note this becomes less useful if the constructor was made explicit

  26. Something other people haven’t pointer out:
    operator + is actually operator +=. That is, the following code:

    Complex a(1, 0), b(0, 1);
    a + b;

    Actually changes the value of a.

    I would probably implement operator + like this:

    complex operator +(complex other) const {
    return complex(real+other.real, imag+other.imag);
    }

    Or perhaps in terms of an implemented += operator.

    I wouldn’t necessarily take arguments by const& since this is a relatively small class and it may not be necessary.

  27. class complex {
    public:
        complex( double r, double i = 0.0 )
            : real(r), imag(i)
        { }
    
        complex operator+ (const complex& other ) const {
            return complex(real + other.real, imag + other.imag);
        }
    
        ostream& operator<<( ostream& os ) const {
            return os << "(" << real << "," << imag << ")";
        }
    
        complex& operator++() {
            ++real;
            return *this;
        }
    
        complex operator++( int ) {
            auto temp = *this;
            ++real;
            return temp;
        }
    
        // ... more functions that complement the above ...
    
    private:
        double real, imag;
    };
  28. @Kris:

    Should `operator+()` and `operator++(int)` return `complex` as `const` instead?

    I don’t think explicit is necessary because implicit conversion isn’t possible with multi-parameter constructors.

  29. operator+ seems to be a typo, since it changes the object – it probably was intended to say operator+=. In that case a reference to *this should be returned, rather than void. If it really is intended to be operator+, it should be const and not change the object, instead it should return a temporary that is the sum.

    The member operator << is a poor choice. As written, you would have to write "complex(1,2) << std::cout" instead of the more natural "std::cout << complex(1,2)". It would be better to make a free-standing operator<< that reverses the operator order. When doing that, an ostream& should be returned instead of void. Also, the parameter should be an ostream&, not an ostream.

    std::ostream is written as just ostream. This requires a preceding "using std::ostream;" or "using namespace std;". Putting that in a header puts that using declaration into all code that includes the header. Whether that is OK depends on the local coding convention, though I'd avoid it. As a minor point, the strings that are being written to os are all one character long, so they could as well be chars. That probably won't matter much, but writing '(' is no more complicated than "(", so you might as well choose the one that's likely to be faster.

    operator++() should return a complex&.

    All of the parameters and the local variable temp in operator++(int) could be const. So a coder following the rule "if it can be const, it should be const" would make them const. However, reasonable people can disagree about whether taking consting to this level makes sense. I would do it.

    A more philosophical note is whether incrementing a floating point complex number is a natural notion. It isn't to me so I'd prefer there not to be an operator++.

  30. 1. Operator + should take take the arg by const ref (pointed out in the prior response)
    2. Pre-increment should return a reference.
    3. operator<< takes an ostream by value — change to ref (also, would be better if the ouput method were moved out of the class)
    4. I would also add a move constructor. The post increment operator is returing a complex by value, a temporary can be eliminated here using the move ctor.

  31. * first of all should have used `std::complex’ instead of implementing their own
    * constructor should be `explicit’ to avoid implicid (unexpected) conversions
    * `operator+()’ should return `complex’, take the argument by const reference and be declared as `const’

        complex operator+ ( const complex& other ) const {
            return complex(real + other.real, imag + other.imag);
        }
    

    * stream operator should be a friend free function returning `std::ostream’ and taking a const reference to `complex as the second argument

        friend
        std::ostream& operator<<( std::ostream& os, const complex& c ) {
            return os << "(" << c.real << "," << c.imag << ")";
        }
    

    * both increment operators do not make much sense, but leaving this aside:
    ** pre-increment `operator++()’ should return reference to `complex’
    ** post-increment `operator++(int)’ ideally should be implemented in terms the former one

        complex operator++( int ) const {
            return ++complex(*this);
        }
    
  32. A few thoughts on first sight:
    – let’s wrap this in a namespace, we wouldn’t want a clash with some other class named “complex”
    – let’s make it a class template instead of hard-coding the underlying type “double” (for “real”, “imag”, “r”, “i”, etc.)
    – optional: possibly use compile-time constraint on the underlying type, for instance, using type-trait like “std::is_floating_point” (or at least “std::is_arithmetic”) coupled with static_assert and/or std::enable_if // although we’re facing some trade-offs here — for instance, we could like to also use Boost.Multiprecision types (which is one of the reasons for making “complex” a class template in the first place)
    – use value-initialization instead of hard-coding “double i = 0” default argument (with unnecessary implicit conversion)
    – operator+ — result: make it return the result (by const-ref), use += for the components (or, even better, implement it in terms of operator += working for the class);
    – operator+ — arguments: I was about to say something about using const-ref, but then I recalled N3445 and thought pass-by-value might be fit better here:
    http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3445.html
    – operator<< — make it behave like the operator<< in the Standard IOStream (take-and-return stream by reference)
    – (prefix) operator++ — there's an avoidable (by reference) copy in the pre-increment operator++
    – (postfix) operator++ — implement in terms of the prefix operator++ instead of handcoding ++real

Comments are closed.