Beefy Boxes and Bandwidth Generously Provided by pair Networks
Welcome to the Monastery
 
PerlMonks  

Mutator chaining considered harmful

by Aristotle (Chancellor)
on Dec 28, 2004 at 23:09 UTC ( #417872=perlmeditation: print w/ replies, xml ) Need Help??

[ Update: let me retrospectively preface this rant with a note that I used to a huge fan of the technique I am deprecating. My disdain has grown organically. There is no single particularly striking reason why it is bad; I have just experienced that trying to work with code that exploits this technique is a severe pain in practice for a number of subtle reasons that combine to bite you, never fatally, but persistently. ]

When seeking advice about your OO Perl style, you may find people from time to time who will encourage you to write mutators (aka setters) such that they will return $self:

sub set_frob {
    my $self = shift;
    my ( $val ) = @_;
    if( @_ == 1 and $val !~ /\D/ ) {
        $self->{ frob } = $val;
    }
    else {
        die $some_param_inval_explanation;
    }
    return $self; # this is all that matters to us
}

A number of popular modules like Tk and SOAP::Lite use this kind of interface. This, proponents argue, will allow you to conveniently chain mutator calls on a single object:

$frobnitzer->set_frob( 42 ) ->set_flub( 712 ) ->set_name( 'veeblefetzer' );

Please don't.


First of all, this creates temptation to mix in calls to setters with method calls with actual return values:

my $label = $button->border( 20 )->label;

This can be pretty confusing to read. Even if you don't think so at this point, this is not at all uncommon in practice, with modules like Data::Dumper actively promoting this kind of interface.

But it gets a whole lot worse yet when the object in question actually aggregates others and lets you ask for them. This introduces multiple different kinds of possible call chains:

$window->title( 'foo' )->border( 20 ); # semantically very different from: $window->child->border( 20 );

Suddenly the observer has to read carefully to keep affairs straight.

In the worst case, mixing all possible scenarios, you get code like this:

my $label_formatting = $window->title( 'foo' ) ->border( 20 ) ->child->border( 20 ) ->child->font_style;

If that's not ugly as all sin, you tell me what is. Transgressions like this are rare, to be sure (and thankful).

But $self-returning mutators blur the lines between types of methods and types of call chains, and ultimately they'll make your code harder to read.

[ Update: Separate things should be kept separate:

$window->title( 'foo' ); $window->border( 20 ); my $button = $window->child; $button->border( 20 ); my $label_formatting = $button->child->font_style;

This is more code, but it is worlds apart in clarity.

[ Update: Note how this codes makes it possible to discriminate between errors from the one mutator vs those from another mutator. In the chained calls version of the code, doing so would require many more provisions. And how do you handle error signalling by the return value of a method? You can't. There are tricks like returning a magic object that can have any method called on it and overloads its boolean value to be false, but… where's that smell coming from? ]

If you're writing a class, please resist the temptation to facilitate chained mutator calls. If you are looking for a sensible return value for your mutators, return the current value of the property, which is analogous to how $foo = $bar = $baz works. ]

[ Update: but better yet, don't write separate mutators per property at all. Write a single method that can update any number of properties in one fell swoop:

$window->set( title => 'foo', border => 20, );

As a bonus, you get to avoid writing repitiuous code or harrowing with code generation schemes, and all your input validation will naturally converge on a single spot in your code without the additional effort this would require with the separate mutators model. ]

Makeshifts last the longest.

Comment on Mutator chaining considered harmful
Select or Download Code
Re: Mutator chaining considered harmful
by eric256 (Parson) on Dec 28, 2004 at 23:23 UTC

    Perhaps is my past with VB but

    my $label_formatting = $window->title( 'foo' ) ->child->border( 20 ) ->child->font_style;

    Makes a huge amount of sense. You want to set the windows title, grab its kid, set its border, grab the kids kid, and get its font_style. While I wouldn't recommend grouping the getter with all the rest, I think its a fine style, if its what is needed. Sometimes you don't want tons of variables when a simple call like that will work. Just as long as the documentation makes it obvious what the methods do I don't see why anyone should complain. To each his own style, if you don't like it, don't use it. If you are going to recommend agianst it, I for one would like you to back it up with more than this.


    ___________
    Eric Hodges
      What kind of kid would the 'foo' title have? Would a window even have multiple titles? That code reads like "Get the child of the title currently named 'foo' from $window where the border is already 20. From there get font style of that object's child." There isn't anything in that chain that is a mutator if you read it using perl5 or VB eyes.

        I don't disagree that it can be misread. Anything can be misread. If you are used to mutators that return the object they mutate, then there is nothing confusing about it. If you are not used to that style then it is confusing. That is no different than other styles. With any style different from your own you will have to learn it. Saying that you don't read it correctly because you don't use that style doesn't mean the style is bad.

        There are however some reasons that style is bad. Mostly it makes it difficult to catch errors if those methods fail. A simple although not elegant solution to that would be to store an error code or message in the object when an error occurs. I'm not recommending that or this style. I just think a post saying this style is bad should include more critism than "I don't like it." and "it's hard for me to read" (not exact quotes, just the feeling i get.)

        If that is not how I should read your argument than I would appreciate a better explanation of why its bad. If not for myself then for others who read this node. Like I said, I don't think this is the best style all around but there are cases where it is usefull.


        ___________
        Eric Hodges
Re: Mutator chaining considered harmful
by perrin (Chancellor) on Dec 29, 2004 at 00:20 UTC
Re: Mutator chaining considered harmful
by diotalevi (Canon) on Dec 29, 2004 at 08:10 UTC

    You've sold me on it. I wasn't able to read a single example and know which ones were merely repeated method calls to the same object and which were successively deeper objects. In fact, I know that all the SOAP examples come with cascaded method calls but I never knew that in reality it wasn't just some deeply nested object being worked with. I always thought that, actually. That's because all the examples used the syntax for working with deep objects - ala A->B->C->D.

    If you can write code that looks really damn legible and still completely throw me on what it means, its a bad idea. There isn't anything I can do to distinguish chained self-returning methods and chained successivly deeper object returning methods. This is just entirely bad practice because it is unreadable. If a person really means to call multiple methods against the same object, there are nice functional ways to do this that don't look like something else. multiple method calls against the same object, revisited has perfectly reasonable syntax to support this desire and it isn't going to be completely obfuscatory. Given my opinions on this, I think a programmer would have to be irresponsible to use chained self-returning method calls.

    my $window = call_on_obj( Gtk2::Window->new( "toplevel" ), [ signal_connect => ( delete_event => sub { Gtk2->main_quit } ) +], [ set_title => "Test" ], [ set_border_width => 15 ], [ add => call_on_obj( Gtk2::Button->new( "Quit" ), [ signal_connect => ( clicked => sub { Gtk2->main_quit } ) ], ) ], [ 'show_all' ], );

      Heh. Spare yourself the sarcasm, I haven't used that in real code yet and don't intend to either. Sorry — I really thought this was dripping sarcasm. It has led me to see a pattern which should have been obvious in retrospect, though. Please see my update on the root node.

      Makeshifts last the longest.

        I wasn't being sarcastic about using the call_onobj function. I wouldn't have written that specific set of function calls because I don't use Gtk2. I used something similar in some code just yesterday. This is off the top of my head and I didn't use your specific function but it did the same sort of thing. In my case I'm effectively generating my functions and parameters by parsing a file and returning a list of events and parameters for them. It so happens that the general technique of writing your function calls as data is really convenient when parsing stuff. I can take one pass over the data to get my list of events and then just execute the events as code.

        call_onobj( $renderer, [ 'UseFont', 0 ], [ 'SetDots', 52 ], [ 'SetScanLines', 300 ], [ 'AdjustDots', 15 ], [ 'AdjustScanLines', 2 ], [ 'Text', 'This is some text to be added to the output ] );

        The original query was on a series of hardcoded method calls all onto the same object. This is more general because now the method calls are mutable as data. It benefits two ways - by being more powerful and by not being a mimic for an unrelated sort of thing. I read $obj->UseFont( 0 )->SetDots( 52 )->SetScanLines( 300 )->AdjustDots( 15 )->AdjustScanLines( 2 )->Text( '...' ) as more likely to be a deep object access than anything else. In general, I suspect that I will continue to be surprised when someone writes the preceding and they are all really just successive method calls on the $obj object. I grant that some uses of whitespace can make the chained-self intent more visible to someone who expects that. I don't think it helps anyone who didn't already have this technique in mind. I also think that no only I not be helped, that I will be hindered. The technique with this syntax is a dead end in perl5 and I would like it for people to write concise, clear code without it.

Re: Mutator chaining considered harmful
by toma (Vicar) on Dec 29, 2004 at 09:13 UTC
    Chained mutators can
    • Save typing
    • Create symmetry
    • Result in clearer code.
    Here's a snippet from my own code. This probes a real-time audio application with a five channel OpenGL oscilloscope:
    my $s= Scope->new('probe' => $netlist) ->anatrace('in', 'System input', 'active', 1) ->anatrace('out_1', 'Lpf bank1', 'active', 1) ->anatrace('out_2', 'Lpf bank2', 'active', 1024) ->anatrace('out', 'System output', 'active', 1) ->anatrace('bank2', 'Switch pos', 'active', 1) ->generate();
    Want to see another channel? Call anatrace again.

    I don't think that features with potential downsides should be 'considered harmful'.

    It is possible to create a 'little language' out of chained method calls. I avoid creating little languages because I have seen a few of them grow into voracious blood-sucking monsters. I can enjoy chaining method calls without fear of triggering a science-related memetic disorder.

    It should work perfectly the first time! - toma
      This gives you almost nothing in comparison with:
      ... $s->anatrace('bank2', 'Switch pos', 'active', 1); $s->generate();
      And when the objects have long names or are expressions you can always:
      for (Scope->new('probe' => $netlist)) { $_->anatrace('in', 'System input', 'active', 1); ... $_->generate(); }
      --kap

      Apart from kappa's points: read my root node update. Here's what that would look like with a good interface:

      my $s = Scope->new( 'probe' => $netlist ); $s->anatrace( in => [ 'System input', 'active', 1 ], out_1 => [ 'Lpf bank1', 'active', 1 ], out_2 => [ 'Lpf bank2', 'active', 1024 ], out => [ 'System output', 'active', 1 ], bank2 => [ 'Switch pos', 'active', 1 ], ); $s->generate();

      Makeshifts last the longest.

        I suppose I could also use this :-)
        my $s = Scope->new( probe => $netlist ) ->anatrace( in => [ 'System input', 'active', 1 ], out_1 => [ 'Lpf bank1', 'active', 1 ], out_2 => [ 'Lpf bank2', 'active', 1024 ], out => [ 'System output', 'active', 1 ], bank2 => [ 'Switch pos', 'active', 1 ], ) ->generate();
        The => after the first arguments look nice, especially since the first arguments are instance handles, and I use them as a hash key.

        In this example the arguments to anatrace really aren't mutators, they are constructors of sub-objects. They are waveforms being added to a scope display.

        update
        Is the problem that you perceive just with mutators, or does it also apply to sub-object constructors? Can you provide examples of acceptable method chaining, and contrast it with problematic method chaining?

        It should work perfectly the first time! - toma
Re: Mutator chaining considered harmful
by Anonymous Monk on Dec 29, 2004 at 10:00 UTC
    The advantage of returning $self is that the user of the code is free to decide to use either style, that is:
    $window->title('foo'); $window->border(20);
    Or
    $window->title('foo')->border(20);
    If you don't return $self, you're taking the decision for the user. Which isn't very friendly.

    Now, I don't have a problem with

    $window->title('foo')->border(20);
    If I see this code, and I've no idea what the title and border methods are, my assumption will be that the title of the window is set to foo, and the border will be 20 pixels wide. I don't see the problem either, unless you have windows with labelled titles, so that there's the possibility that $window->title('foo') returns the 'foo' title.

    I probably won't do the chaining as you give. I'd probably write it as:

    $window->title('foo')->border(20); my $button = $window->child->border(20); my $label_formatting = $button->child->font_style;

    But I will return $self from accessors.

      If you don't return $self, you're taking the decision for the user. Which isn't very friendly.

      That's actually a better argument than it might seem like, but, as I wrote in my root node update, if you don't write separate mutators, but rather a single central property setter method, you completely obviate the need for chaining to begin with:

      $window->set( title => 'foo', border => 20 );

      And I was almost going to point out that you were picking up the wrong widget in your rewrite of the chaining because the en passant assignment in

      my $button = $window->child->border(20);

      misled me.

      Really, don't mix these things. It seriously makes reading the code an excercise in frustration.

      Makeshifts last the longest.

        Let's compare:

        $window->title('foo')->border(20); # versus $window->set(title => 'foo', border => 20);

        Clearly, I like method chaining. I use it frequently and my primary argument to people is also "don't use it if you don't want it." However I would never encourage method chaining if the mutator didn't return an object of the same class as the object that received the message (or maybe with an isa relationship and with constructors being a special case). That's just begging Demeter to beat you to death. However, when I look at your code, I see one great big "catch-all" mutator where now I no longer can merely validate the values, but I also have to validate the names of the attributes. It would be easy to type this:

        $window->set(title => 'foo', border => 20);

        Depending on how someone codes that, it could easily silently fail when a direct method call would not.

        I suppose we could argue all day long about which technique is better, but both of our preferred styles have strengths and weaknesses. For now I'll continue to return $self and you can continue to ignore it :)

        Cheers,
        Ovid

        New address of my CGI Course.

Re: Mutator chaining considered harmful
by dragonchild (Archbishop) on Dec 29, 2004 at 14:24 UTC
    In reading a completely unrelated article, I found this snippet:

    Interaction-based testers do talk more about avoiding 'train wrecks' - method chains of style of getThis().getThat().getTheOther(). Avoiding method chains is also known as following the Law of Demeter. While method chains are a smell, the opposite problem of middle men objects bloated with forwarding methods is also a smell. (I've always felt I'd be more comfortable with the Law of Demeter if it were called the Suggestion of Demeter.)1

    I found it interesting that Fowler links method chaining to users of the state-based testing style. The fact that interaction-based testers view chaining to be a 'train wreck' was also telling.

    No real point to this ... just found it interesting. :-)

    1. From Mocks Aren't Stubs by Martin Fowler

    Being right, does not endow the right to be rude; politeness costs nothing.
    Being unknowing, is not the same as being stupid.
    Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
    Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

      Of course those are aggregate object calling chains, which aren't the same as the mutator chains I talk about.

      But yeah, there's a lot of different bluffs and cliffs to steer around in the area.

      Makeshifts last the longest.

Re: Mutator chaining considered harmful
by fizbin (Chaplain) on Dec 29, 2004 at 22:18 UTC
    (Pulling this from the cb)

    You know, there's only one place where I've come to think that self-chaining of mutators is a good idea, and that's in things like java's StringBuffer.append: basically, when the manipulator method is an accumulator of some sort.

    So I've seen self-chaining, for example, in a C++ class that did checksums:

    Checksum myCS; myCS.addTo(x).addTo(foo).addTo(barObject); cout << myCS;
    Or the more familiar: (again, C++)
    cout << "Value is: " << dec << myValue << " (hex: " << hex << myValue +<< ")";
    Here "dec" and "hex" aren't manipulators in the same sense as above, quite, but the syntax is clearly very similar. Again, though, the sense is that << is acting as an accumulator, and what passes through it adds on to what was there before, instead of being a "setter" method.

    So getting back to Perl, this is in my opinion a perfectly respectable interface for a file-parsing library:

    my $Ifile_format = new What::Ever::Format(); $Ifile_format->addRecType($XErec) ->addRecType($XFrec) ->addRecType($I7rec) ->addRecType($HMrec);
    In this case, addRecType, while changing the state of $Ifile_format, is adding on new potential record types, and not setting some distinct value.
    -- @/=map{[/./g]}qw/.h_nJ Xapou cets krht ele_ r_ra/; map{y/X_/\n /;print}map{pop@$_}@/for@/
      Or to stay closer to home, Perl's 5.9.x stacking file operators:
      if (-r -w $file) { # File is readable and writeable ... }

        I don't think those are stacked, exactly. I think they use the syntax of single-letter command-line options.

        -vH

      Echoing the point made in the updated OP, wouldn't it be better to create a method that let you do that all at once?
      $Ifile_format->addRecTypes($XErec, $XFrec, $I7rec, $HMrec);

      “For the record,” I agree. I originally ++ed this node but for some reason never commented.

      Makeshifts last the longest.

Re: Mutator chaining considered harmful
by Anonymous Monk on Dec 30, 2004 at 02:10 UTC
    I wrote about this in my use.perl journal at http://use.perl.org/~Revelation/journal/12867.

    Looking back, I had to major problems (that weren't all that well articulated):
  • Chained method calls make backwards compatibility hard to do, if you make an attribute into its own object at any time. You can't use Want.pm, because either way you want an object.
  • Chained method calls force a person reading to check whether a mutator has returned the same object or a more specific object.

    I agree with you. Not a big fan.

      Chained method calls make backwards compatibility hard to do, if you make an attribute into its own object at any time. You can't use Want.pm, because either way you want an object. You mean, it makes it difficult to change a method such that has a different return value? Well, that always is non-backwards compatible. Whether the user chains, or stores the result in a variable and does something with the variable, it's going to be non-backwards compatible.

      Chained method calls force a person reading to check whether a mutator has returned the same object or a more specific object. And that's different from storing the result of a method (whether said method is a mutator or something else) in a variable, and call a method in said variable?

        Re: And that's different from storing the result of a method (whether said method is a mutator or something else) in a variable, and call a method in said variable?

        Yes, it is. Consider:  $people->bob->eye('green')->color; . As a reader, I assume that the call is returning an 'eye' object, not bob, again. But if eye's a mutator, then you might find out Bob's a brown man.

        Re: You mean, it makes it difficult to change a method such that has a different return value? Well, that always is non-backwards compatible. Whether the user chains, or stores the result in a variable and does something with the variable, it's going to be non-backwards compatible. In most cases you can make it backwards compatible through Want.pm. But perhaps, this is an issue of my style...?
Re: Mutator chaining considered harmful
by itub (Priest) on Jan 01, 2005 at 17:25 UTC
    "Considered Harmful" Essays Considered Harmful ;-)

    Seriously, there's something I find questionable. You show how chained mutators can be abused and dangerous, and the solution is "please don't" write the mutators to support this usage (unless I misunderstood and the "please don't" only applied to doing the chaining, rather that writing the mutator that way). Going back to the famous "goto considered harmful essay", that would be like saying that, since goto can be abused, languages shouldn't have a goto. While this is a nice purist position to take, decades after the essay many real-life, pragmatic language designers include goto anyway and leave the responsibility of using it wisely (if needed) to the user. I choose to do the same: I allow the mutators I write to be chained, even if in practice I only chain them in limited cases where they are really clear (in my opionion).

    Of course, if you really want to return an error code from your mutators, please go ahead and be consistent. I think returning the value that was set is useless, as others have said in this thread. But if the choice is between an undefined return value and returning $self, why not return $self and let the user do whatever he or she wants with it?

      Yes, I suggested that class authors not return $self from mutators. Of course, as the discussion in multiple method calls against the same object, revisited shows, there are ways to retrofit chaining from the user's end to classes that don't implement it (I was almost going to upload one of those to CPAN).

      So I don't see a valid case for returning $self as an enabler of choice: the choice is always available, regardless of the classes interface. What's left then is the question of encouraged usage style (see the docs to SOAP::Lite). And to me, there's no question that this style should not be encouraged, just as there's no question that use of goto should not be encouraged, regardless of one or the other is sometimes the right tool or not (for which there is clear evidence in case of goto; with mutator chaining I've yet to see any).

      The only reason to favour chainable mutators I've seen anyone mention is brevity, conciseness. But as a class author you can offer that to users without conflating semantics by offering a single unified setter.

      Makeshifts last the longest.

Re: Mutator chaining considered harmful
by Anonymous Monk on Jan 04, 2005 at 03:18 UTC
    I too used to think that chaining mutants was a good idea. They just seem so dangerous running around among us.

    But I've learned from painful experience that this is a very dangerous practice and must encourage everyone...

    Please don't do this!

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://417872]
Approved by VSarkiss
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others chilling in the Monastery: (5)
As of 2014-11-23 21:37 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My preferred Perl binaries come from:














    Results (134 votes), past polls