tags:

views:

1136

answers:

20

As a rule, How many function chains per line is too complicated?

By Function chaining I mean:

Obj.Display().Show().Result()

What does your team use a guideline?

+10  A: 

However many - as long as it remains readable.

Michael Burr
Readability is important, but it is as much important not to require services from object without explicit dependencies, so, unless this represents a fluent interface use case, I would consider it a code smell even at two chained invocations.
Jacek Szymański
+2  A: 

As a rule, whatever you and your other dev team members can tolerate.

curtisk
+2  A: 

Until you are not sure what the last object is.

Robert
+11  A: 

Umm, 42. Yeah thats it 42.

Dave Ganger
+11  A: 

I think two is too many. Code is a lot more readable and robust if you actually show what those functions are returning, maybe do some validation to check for null, etc, and then call the next function.

Code will be read many, many times more often than it will be written. Verbosity can be a good thing.

Eric Z Beard
By "two is too many" do you mean Obj.Display.Show() is too many (two method invocations), or Obj.Display.Show().Result() is too many (two chains)?
AaronSieb
It's just an instinctive code review reaction I have when I see "X.Y.Z". I think, wait, what is Y? Could it be null?
Eric Z Beard
+2  A: 

It depends to some degree on the language, too. While it may be "bad form" to do a long chain in, say, Java, it's considered good fun in Ruby.

Pesto
+3  A: 

It depends, but usually when reusing parts of the chain it can be easier to read if cached:

var s = Obj.Display().Show();
s.Result();
s.SomethingElse();

Instead of:

Obj.Display().Show().Result();
Obj.Display().Show().SomethingElse();
dalle
+1  A: 

I think more than two is stretching it. The reason is, if any of the methods happen to return null and a NullPointerException (or equivalent) is thrown, the stack trace does not point you straight to the bug.

I suppose I could make exceptions for cases when the methods are documented to never return null, honor that contract, and can be proven to honor that contract. In general, fewer is better, IMHO.

Brandon DuRette
+6  A: 

Unless it is a fluent interface, two is too many. It is because if you need to do such a thing, this probably means you need some service from an object you have no explicit dependency on.

Fluent interfaces are different because you are chaining method calls to the same object or a group of objects designed for such an usage.

EDIT: correction about fluent interfaces

Jacek Szymański
Actually, for most Fluent interfaces I've seen, you are no chaining to the same object. Usually, the first step creates a internal use object (who's type is rarely known to the user), and the next step pass that, until one changes it to a different internal object.
James Curran
+3  A: 

I may be the only one here, but personally I really hate method chaining. For each dereferencing of a result value I see possible null pointers and exceptions thrown mid-stream. And often enough there is no catch block anywhere to help with either of those.

In C/C++ your code will not even throw an exception but crash on a NULL pointer.

Thorsten79
You're definitely not the only one. Every time I have to debug one of these statements, I end up decomposing it into multiple independant calls, with temporary variables.
Mark Bessey
A: 

Chaining is okay as long as you as know what you are doing and what can happen in a bad case. I personally never like to wrap everything in a try/catch-block.

unexist
A: 

I have a personal preference that is limited by number of characters, not number of calls.

I try to cap my line length at 80-110 characters which, by definition, limits the number of chained method calls I can make.

That being said, chained method calls unwittingly add complexity and make it harder to debug your code.

Read up on the Law of Demeter. Chaining/complexity leads to trouble.

link text

What about splitting your line among many lines like this posthttp://stackoverflow.com/questions/154864/function-chaining-how-many-is-too-many#155443
Brian G
+11  A: 

The Law of Demeter suggests you should use only one dot for maintainability. The upside is that you enforce better encapsulation if the inner methods aren't exposed. The downside is that you end up writing more wrapper methods.

Nick Hebb
Nice and succinct. As with anything, it's not black and white and I often break the law in the name of pragmatism.
Mal Ross
+2  A: 

I personally like to follow 2 rules on chaining:

  1. If results can be cached and reused, do so. (see post by dalle.myopenid.com)
  2. If the line will not fit on the screen without word wrapping or scrolling, break it into smaller parts for readability.
chills42
A: 

Shouldn't you be using getters and setters here anyway, so:

Obj.Display.Show.Result

Otherwise, the problem I have with this code is that your function names are verbs, but they don't describe the return type, so it is harder to read. Would it be better if the function names had a verb and a noun, something like:

Obj.createDisplayBox().createShowWindow().addResult();

I think this many dots would be acceptable if you didn't intend to reuse any of the returned objects in the chain

Iain
It is general practice to still keep methods in certain cases. For example, if it takes a long time to calculate or some other reasons. FxCop rules have a good list...
Mufasa
+3  A: 

One good programming practice is to step through everything you write in the debugger. As you do this, function chaining makes it hard to see what intermediate values get produced. Of course you can step into each call, but that's slower, and even then you might not find a variable to examine. (Some debuggers let you look at return values using a special symbol, but not all of them do.)

Similarly, if you're debugging a problem, it's much harder to select between step-into and step-over if you're in the middle of a big function chain. I always hit the wrong one and then have to start over, or else go set an extra breakpoint that will annoy me later.

So if you use an interactive debugger then it can be easier to be verbose and write some intermediate temporary variables than to do any chaining at all.

Eric
A: 

Using resharper I can pretty much mouse-over any part to know more about it, so I'm comfortable writing a few chains and you won't hear me whining about not knowing the types involved, but just not repetitive ones like:

Obj.Display().Show().Result();
Obj.Display().Show().ResultExpanded();

Now, with some of our objects the code ends up looking more like:

RegInfo.SiteID = int.Parse(ConfigManager.getInstance().getConfig().getSingleProperty(Constants.SITEID_KEY));

[not my code] Sometimes when it's really long it gets broken into bits:

RegInfo.SiteID = int.Parse(
  ConfigManager.getInstance().getConfig()
  .getSingleProperty(Constants.SITEID_KEY)
);
dlamblin
+8  A: 

As others have pointed out, it depends on the nature of your methods. If there's a chance that any of the method calls can fail, don't chain them. However, if the API is specifically designed for safe method chaining, chain them as much as you like.

The Hibernate Criteria API is a good example of where extreme chaining is safe and actually makes the code more readable -- provided that it's nicely indented. Here's a sample from http://www.hibernate.org/hib_docs/v3/api/org/hibernate/Criteria.html:

 List cats = session.createCriteria(Cat.class)
     .setProjection( Projections.projectionList()
         .add( Projections.rowCount() )
         .add( Projections.avg("weight") )
         .add( Projections.max("weight") )
         .add( Projections.min("weight") )
         .add( Projections.groupProperty("color") )
     )
     .addOrder( Order.asc("color") )
     .list();

Also note that in C++, STL streams rely on method chaining to allow the following syntax:

cout << "All your base are belong to " << strWhom << endl;

Needless to say, it's absolutely OK to use extensive chaining here; in fact, it makes the reading of the code more natural. I don't think there's a general rule for the maximum number of chains beyond which code would be rendered unreadable. It all depends on the context.

Ates Goral
A: 

simple rule.. if it's unreadable, it's too many.

Jeremy Cantrell
+1  A: 

The readability rule is definitely a good start. On the last team I worked on several of the developers were greatly confused by such method chaining and as a result I was forced to rewrite my code so that they could read it. Try out different examples then have various team members read it, .NET LINQ example might be a good start or any other fluent interface. If they are having trouble reading it or after several attempts to share it with them it is still being rejected then you may want to keep your chaining to a minimum. Try to strike a balance and good luck.

smaclell
I agree regarding fluent interfaces. They are quickly becoming my pattern of choice for factory and constructor interfaces, instead of using smelly setters.
moffdub