Friday, April 8, 2011

Method chaining + inheritance don't play well together?

Consider:

// member data omitted for brevity

// assume that "setAngle" needs to be implemented separately
// in Label and Image, and that Button does need to inherit
// Label, rather than, say, contain one (etc)

struct Widget {
    Widget& move(Point newPos) { pos = newPos; return *this; }
};

struct Label : Widget {
    Label& setText(string const& newText) { text = newText; return *this; }
    Label& setAngle(double newAngle) { angle = newAngle; return *this; }
};

struct Button : Label {
    Button& setAngle(double newAngle) {
        backgroundImage.setAngle(newAngle);
        Label::setAngle(newAngle);
        return *this;
    }
};

int main() {
    Button btn;

    // oops: Widget::setText doesn't exist
    btn.move(Point(0,0)).setText("Hey");

    // oops: calling Label::setAngle rather than Button::setAngle
    btn.setText("Boo").setAngle(.5); 
}

Any techniques to get around these problems?

Example: using template magic to make Button::move return Button& or something.

edit It has become clear that the second problem is solved by making setAngle virtual.

But the first problem remains unsolved in a reasonable fashion!

edit: Well, I guess it's impossible to do properly in C++. Thanks for the efforts anyhow.

From stackoverflow
  • Well, you know it's a Button so you should be able to cast the returned Widget& as a Button& and keep going. It does look a bit ugly though.

    Another rather annoying option is to create a wrapper in your Button class for the Widget::move function (and friends). Probably not worth the effort to wrap everything if you have more than a handful of functions though.

    lc : Exactly, so you're probably just better off calling them one at a time.
    strager : Chaining makes code easier to read? Not in this case, in my opinion!
  • [rant]

    Yes. Quit this method chaining business and just call functions in a row.

    Seriously, you pay a price for allowing this syntax, and I don't get the benefits it offers.

    [/rant]

    Bill Karwin : Perhaps people are trying to conserve semicolons? There's only so many of them you know. ;-)
    jalf : Or you could define a proper constructor which takes all the needed parameters in a single line. :)
    strager : @jalf, NO! @Iraimbilanja, Imagine a function which calculates pi in one line. Now imagine one which does the same in 20. Which one is better in the long run?
    jalf : no? You *prefer* having half-constructed objects littered over your codebase? If the object needs to be positioned before it's valid, then it should be given a position in the constructor.
    strager : @jalf, I come from a Qt background, where constructors were very light. Why would an object need to be assigned a position to be valid? A default position is usable (0, 0), as well as default text (""), font (system), orientation (0 degrees), etc. A constructor makes the object, and that's all.
    jalf : WinForms constructors are generally very light too, and it's a terrible practice. It means that after creating the object, you can spend the next half page of code actually initializing it with meaningful values. Which is exactly what constructors are meant for.
    jalf : "Empty" is not *always* a valid state. For some objects it is, but certainly not for all. For GUI specifically, what is valid? Is it when the app looks correct (then empty is not ok)? Or is it merely when it doesn't crash (then empty/blank is fine).
    strager : I completely agree with @Iraimbilanja (thanks for backing me up =]). @jalf, He isn't rewriting or modelling after WinForms. He's making something different, and probably easier to use. Don't use one broken example as a reason to not do something, where there are many examples showing its benefits.
    jalf : But the fact that you try to jump through these hoops just to clean up your GUI initialization is a strong hint that in *your* case, empty is *not* a valid state. ;) If you want to make initialization easier, write a constructor (or a free function doing the same)
    jalf : strager: I'm not using Winforms as an example of anything other than that light constructors can be a major pain in the butt. ;) And the code in this question highlights the exact same thing.
  • Not with C++.

    C++ does not support variance in return types, so there's no way to change the static type of the reference returned from Widget.move() to be more specific than Widget& even if you override it.

    The C++ needs to be able to check things at compile time, so you can't use the fact that what's really being returned from move is a button.

    At best, you can do some runtime casting, but it's not going to look pretty. Just separate calls.

    Edit: Yes, I'm well aware of the fact that the C++ standard says that return value covariance is legitimate. However, at the time I was teaching and practicing C++, some mainstream compilers (e.g., VC++) did not. Hence, for portability we recommended against it. It is possible that current compilers have no issue with that, finally.

    Uri : I'm wondering why this was downvoted...
    Joe Soul-bringer : This is the best answer, it should up voted!
    Michael Burr : C++ does support variance in return types (co-variance anyway, which is what the question is looking for).
    Uri : Covariance is part of C++ standard, but wasn't part of mainstream visual compilers (e.g., Visual C++ 5/6) back when I was teaching C++. So we taught it as unsupported. Not sure how newer compilers handle this
    Michael Burr : You are right that VC6 does not support covariance. All the other compilers I have readily available do.
    Uri : I added the clarification. I've been doing mostly Java for the last 8 years... At the time, everything we used for teaching didn't support it.
  • For the second problem, making setAngle virtual should do the trick.

    For the first one, there are no easy solutions. Widget::move returns a Widget, which doesn't have a setText method. You could make a pure virtual setText method, but that'd be a pretty ugly solution. You could overload move() on the button class, but that'd be a pain to maintain. Finally, you could probably do something with templates. Perhaps something like this:

    // Define a move helper function
    template <typename T>
    T& move(T& obj, Point& p){ return obj.move(p); };
    
    // And the problematic line in your code would then look like this:
    move(btn, Point(0,0)).setText("Hey");
    

    I'll let you decide which solution is cleanest. But is there any particular reason why you need to be able to chain these methods?

  • I think (I haven't tested it) this will do it using templates:

    template<class T> struct TWidget {
        T& move(Point newPos) { pos = newPos; return (T&)*this; }
    };
    
    template<class T> struct TLabel : TWidget<T> { ... }
    
    struct Label : TLabel<Label> { ... }
    
    struct Button : TLabel<Button> { ... }
    

    Notes:

    • Any/every base class needs to be a template, with a separate non-template leaf class at the top (contrast the LabelT and Label classes).
    • The cast could be a dynamic_cast if you like.
    • Instead of casting the "return *this", the base class could contain a T& as a data member (the derived class would pass this to the base class' constructor), which would be an extra data member, but which avoids a cast and I think may permit composition instead of or as well as inheritance.
    j_random_hacker : +1 but a couple of suggestions: (1) Use static_cast<> instead of dynamic_cast<> as it's faster and still safe (trying to cast outside a class hierarchy is a compile-time error); (2) Don't cast each "return *this;", just define a private member function derived() that does so and "return derived()".
  • This compiles on gcc 4.3.2 and is sort of a mixin pattern.

    #include <string>
    
    using namespace std;
    
    struct Point {
        Point() : x(0), y(0) {}
        Point(int x, int y) : x(x), y(y) {}
    
        int x, y;
    };
    
    template <typename T>
    struct Widget {
        T& move(Point newPos) {
            pos = newPos;
            return *reinterpret_cast<T *> (this);
        }
    
        Point pos;
    };
    
    template <typename T>
    struct Label : Widget<Label<T> > {
        T& setText(string const& newText) {
            text = newText;
            return *reinterpret_cast<T *> (this);
        }
        T& setAngle(double newAngle) {
            angle = newAngle;
            return *reinterpret_cast<T *> (this);
        }
    
        string text;
        double angle;
    };
    
    struct Button : Label<Button> {
        Button& setAngle(double newAngle) {
            backgroundImage.setAngle(newAngle);
            Label<Button>::setAngle(newAngle);
            return *this;
        }
    
        Label<Button> backgroundImage;
    };
    
    int main() {
        Button btn;
    
        // oops: Widget::setText doesn't exist
        btn.move(Point(0,0)).setText("Hey");
    
        // oops: calling Label::setAngle rather than Button::setAngle
        btn.setText("Boo").setAngle(0.0); 
    }
    
    ChrisW : I see two problems with this implementation: 1) you can't have a stand-alone Label; 2) Widget::move is returning Label
  • A simple, but annoying, way of solving your problem is to reimplement all your public methods in your subclasses. This doesn't solve the issue with polymorphism (if you cast from Label to Widget, for example), which may or may not be a major issue.

    struct Widget {
        Widget& move(Point newPos) { pos = newPos; return *this; }
    };
    
    struct Label : Widget {
        Label& setText(string const& newText) { text = newText; return *this; }
        Label& setAngle(double newAngle) { angle = newAngle; return *this; }
        Label& move(Point newPos) { Widget::move(newPos); return *this; }
    };
    
    struct Button : Label {
        Button& setText(string const& newText) { Label::setText(newText); return *this; }
        Button& setAngle(double newAngle) {
            backgroundImage.setAngle(newAngle);
            Label::setAngle(newAngle);
            return *this;
        }
        Button& move(Point newPos) { Label::move(newPos); return *this; }
    };
    

    Be sure to include in your documentation this must be done for chaining to work.

    But really, now, why bother with method chaining? Many times, these functions will be called less trivially, and will need longer lines. This would really hurt readability. One action per line -- this is a general rule when it comes to ++ and -- too.

    strager : Make a function, makeButton(float angle, std::string text, int x, int y); Then all that and the addTo(this). Not hard at all!
    strager : @Iraimbilanja, Is this your only reason for using chaining (the code you pasted)? Or are you using it for more complicated things? If so, you can make the makeButton function and just ditch chaining completely.
    peterchen : makeButton (or full constructors) are fine if you have named parameters or about three parameters to initialize. Five or more parameters and chaining is great - especially if you normally initialize only a subset and the subset varies a lot.
  • Is a Button really a Label? You seem to be violating the Liskov substitution principle. Perhaps you should consider the Decorator pattern to add behaviors to Widgets.

    If you insist on the structure as is, you can solve your problem like so:

    struct Widget {
        Widget& move(Point newPos) { pos = newPos; return *this; }
        virtual ~Widget();  // defined out-of-line to guarantee vtable
    };
    
    struct Label : Widget {
        Label& setText(string const& newText) { text = newText; return *this; }
        virtual Label& setAngle(double newAngle) { angle = newAngle; return *this; }
    };
    
    struct Button : Label {
        virtual Label& setAngle(double newAngle) {
            backgroundImage.setAngle(newAngle);
            Label::setAngle(newAngle);
            return *this;
        }
    };
    
    int main() {
        Button btn;
    
        // Make calls in order from most-specific to least-specific classes
        btn.setText("Hey").move(Point(0,0));
    
        // If you want polymorphic behavior, use virtual functions.
        // Anything that is allowed to be overridden in subclasses should
        // be virtual.
        btn.setText("Boo").setAngle(.5); 
    }
    
  • Do setText() and setAngle() really need to return their own types in each class? If you set them all to return Widget&, then you can just use virtual functions as follows:

    struct Widget {
        Widget& move(Point newPos) { pos = newPos; return *this; }
        virtual Widget& setText(string const& newText) = 0;
        virtual Widget& setAngle(double newAngle) = 0;
    };
    
    struct Label : Widget {
        virtual Widget& setText(string const& newText) { text = newText; return *this; }
        virtual Widget& setAngle(double newAngle) { angle = newAngle; return *this; }
    };
    
    struct Button : Label {
        virtual Widget& setAngle(double newAngle) {
            backgroundImage.setAngle(newAngle);
            Label::setAngle(newAngle);
            return *this;
        }
    };
    

    Note that even if the return type is Widget&, the Button- or Label-level functions will still be the ones called.

  • C++ does support return value covariance on virtual methods. So you could get something like what you want with a little work:

    #include <string>
    using std::string;
    
    // member data omitted for brevity
    
    // assume that "setAngle" needs to be implemented separately
    // in Label and Image, and that Button does need to inherit
    // Label, rather than, say, contain one (etc)
    
    
    struct Point
    {
        Point() : x(0), y(0) {};
        Point( int x1, int y1) : x( x1), y( y1) {};
    
        int x;
        int y;
    };
    
    struct Widget {
        virtual Widget& move(Point newPos) { pos = newPos; return *this; }
        virtual ~Widget() {};
    
        Point pos;
    };
    
    struct Label : Widget {
        virtual ~Label() {};
        virtual Label& move( Point newPos) { Widget::move( newPos); return *this; }
    
        // made settext() virtual, as it seems like something 
        // you might want to be able to override
        // even though you aren't just yet
        virtual Label& setText(string const& newText) { text = newText; return *this; }
        virtual Label& setAngle(double newAngle) { angle = newAngle; return *this; }
    
        string text;
        double angle;
    };
    
    struct Button : Label {
        virtual ~Button() {};
        virtual Button& move( Point newPos) { Label::move( newPos); return *this; }
        virtual Button& setAngle(double newAngle) {
            //backgroundImage.setAngle(newAngle);
            Label::setAngle(newAngle);
            return *this;
        }
    };
    
    int main()
    {
        Button btn;
    
        // this works now
        btn.move(Point(0,0)).setText("Hey");
    
        // this works now, too
        btn.setText("Boo").setAngle(.5); 
    
      return 0;
    }
    

    Note that you should use virtual methods for doing something like this. If they aren't virtual methods, then the 'reimplemented' methods will result in name-hiding and the method called will depend on the static type of the variable, pointer or reference so it might not be he correct method if a base pointer or reference is being used.

  • For a while there I thought that it might be possible to overload the slightly unusual operator->() for chaining methods instead of ., but that faltered because it seems the compiler requires the identifier to the right of the -> to belong to the static type of the expression on the left. Fair enough.

    Poor Man's Method Chaining

    Stepping back for a moment, the point of method chaining is to avoid typing out long object names repeatedly. I'll suggest the following quick and dirty approach:

    Instead of the "longhand form":

    btn.move(Point(0,0)); btn.setText("Hey");
    

    You can write:

    {Button& _=btn; _.move(Point(0,0)); _.setText("Hey");}
    

    No, it's not as succinct as real chaining with ., but it will save some typing when there are many parameters to set, and it does have the benefit that it requires no code changes to your existing classes. Because you wrap the entire group of method calls in {} to limit the scope of the reference, you can always use the same short identifier (e.g. _ or x) to stand for the particular object name, potentially increasing readability. Finally, the compiler will have no trouble optimising away _.

    j_random_hacker : Yep, it sure is. But then, you're using C++, what did you expect? :)
  • You can extend CRTP to handle this. monjardin's solution goes in the right direction. All you need now is a default Label implementation to use it as a leaf class.

    #include <iostream>
    
    template <typename Q, typename T>
    struct Default {
        typedef Q type;
    };
    
    template <typename T>
    struct Default<void, T> {
        typedef T type;
    };
    
    template <typename T>
    void show(char const* action) {
        std::cout << typeid(T).name() << ": " << action << std::endl;
    }
    
    template <typename T>
    struct Widget {
        typedef typename Default<T, Widget<void> >::type type;
        type& move() {
            show<type>("move");
            return static_cast<type&>(*this);
        }
    };
    
    template <typename T = void>
    struct Label : Widget<Label<T> > {
        typedef typename Default<T, Widget<Label<T> > >::type type;
        type& set_text() {
            show<type>("set_text");
            return static_cast<type&>(*this);
        }
    };
    
    template <typename T = void>
    struct Button : Label<Button<T> > {
        typedef typename Default<T, Label<Button<T> > >::type type;
        type& push() {
            show<type>("push");
            return static_cast<type&>(*this);
        }
    };
    
    int main() {
        Label<> lbl;
        Button<> btt;
    
        lbl.move().set_text();
        btt.move().set_text().push();
    }
    

    That said, consider whether such an effort is worth the small added syntax bonus. Consider alternative solutions.

    j_random_hacker : +1, nice trick -- you pass the most-derived type as type parameter T, using "void" as a flag to say "actually *this* is the most derived type." But I agree that it's a lot of effort to go...
  • Other people have hit on design issues. You can sort of workaround the problem (albeit in a pretty gross fashion) using C++'s support for covariant return types.

    struct Widget {
        virtual Widget& move(Point newPos) { pos = newPos; return *this; }
    };
    
    struct Label : Widget {
        Label& move(Point newPos) { pos = newPos; return *this; }
        Label& setText(string const& newText) { text = newText; return *this; }
        Label& setAngle(double newAngle) { angle = newAngle; return *this; }
    };
    
    struct Button : Label {
        Button& setAngle(double newAngle) {
         backgroundImage.setAngle(newAngle);
         Label::setAngle(newAngle);
         return *this;
        }
    };
    
    int main() {
        Button btn;
    
        // oops: Widget::setText doesn't exist
        btn.move(Point(0,0)).setText("Hey");
    
        // oops: calling Label::setAngle rather than Button::setAngle
        btn.setText("Boo").setAngle(.5); 
    }
    

    Really though chaining methods the way you are makes for strange code, and hurts readability rather than helps. If you killed the return reference to self junk your code would become:

    struct Widget {
        void move(Point newPos) { pos = newPos; }
    };
    
    struct Label : Widget {
        void setText(string const& newText) { text = newText; }
        void setAngle(double newAngle) { angle = newAngle; }
    };
    
    struct Button : Label {
        void setAngle(double newAngle) {
         backgroundImage.setAngle(newAngle);
         Label::setAngle(newAngle);
        }
    };
    
    int main() {
        Button btn;
    
        // oops: Widget::setText doesn't exist
        btn.move(Point(0,0));
        btn.setText("Hey");
    
        // oops: calling Label::setAngle rather than Button::setAngle
        btn.setText("Boo");
        btn.setAngle(.5); 
    }
    
  • I'd abandon the chaining stuff. For one, it can't actually be done without doing some relatively nasty hacks. But, the biggest problem is that it makes it harder to read and maintain code and you will most likely end up with people abusing it, creating one giant line of code to do multiple things (think back to high-school algebra and those gigantic lines of additions, subtractions and multiplications you always seem to end up with at some point, that is what people will do if you let them).

    Another problem is that because most of your functions in the system are going to be returning a reference to itself, its going to be logical that all of them should. When (not if) you finally do start implementing functions that should return values as well (not just accessors, but some mutators will as well, and other generic functions), you will be faced with a dilemma, either break your convention (which will snowball, making it unclear as to how things should be implemented for other future functions) or be forced to start returning values via parameters (which I'm sure you would loathe, as most other programmers I know do as well).

  • Really, method chaining is a bad idea due to poor exception safety. Do you really need it?

0 comments:

Post a Comment