Is there anything inherently wrong with long object invokation chains?

Question

I've organized my code so that the data is organized hierarchically. I find myself crawling up the tree using code like the following:

File clientFolder = task.getActionPlan().getClientFile().getClient().getDocumentsFolder();

Now, since I'm not drilling down into the task object, I'm drilling up to its parents, I don't think I'm loosing anything in terms of encapsulation, but there's a flag going off in the back of my mind telling me there's something dirty about doing it this way.

Thoughts?

Answer 1

The biggest flag in the world.

You cannot check easily if any of those invokations returns a null object thus making tracking any sort of error next to impossible!

getClientFile() may return null and then getClient() will fail and when you are catching this, assuming you are try-catching you won't have a clue as to which one failed.

Answer 2

How likely is it to get nulls or invalid results? That code is dependent on the successful return of many functions and it could be harder to sort out errors like null pointer exception.

It's also bad for the debugger: less informative since you have to run the functions rather than just watching a few local variables, and awkward to step into the later functions in the chain.

Answer 3

Yes. It's not best practice. For one thing, if there's a bug, it's harder to find it. For example, your exception handler might display a stack trace that shows that you have a NullReferenceException on line 121, but which of these methods is returning null? You'd have to dig into the code to find out.

Answer 4

This is a subjective question but I don't think there's anything wrong with it up to a point. For instance if the chain extends beyond the readable aread of the editor then you should introduce some locals. For instance, on my browser I can't see the last 3 calls so I have no idea what you're doing :).

Answer 5

Well it depends. You shouldn't have to reach through an object like that. If you control the implementations of those methods, I'd recommend refactoring so that you don't have to do that.

Otherwise, I see no harm in doing what you're doing. It's certainly better than

ActionPlan AP = task.getActionPlan();
ClientFile CF = AP.getClientFile();
Client C = CF.getClient();
DocFolder DF = C.getDocumentsFolder();

Answer 6

Well, every indirection adds one point where it could go wrong.

In particular, any of the methods in this chain could return a null (in theory, in your case you might have methods that cannot possibly do that), and when that happens you'll know it happened to one of those methods, but not which one.

So if there is any chance any of the methods could return a null, I'd at least split the chain at those points, and store in intermediate variables, and break it up into individual lines, so that a crash report would give me a line number to look at.

Apart from that I can't see any obvious problems with it. If you have, or can make, guarantees that the null-reference won't be a problem, it would do what you want.

What about readability? Would it be clearer if you added named variables? Always write code like you intend it to be read by a fellow programmer, and only incidentally be interpreted by a compiler.

In this case I would have to read the chain of method calls and figure out... ok, it gets a document, it's the document of a client, the client is coming from a ... file... right, and the file is from an action plan, etc. Long chains might make it less readable than, say, this:

ActionPlan taskPlan = task.GetActionPlan();
ClientFile clientFileOfTaskPlan = taskPlan.GetClientFile();
Client clientOfTaskPlan = clientFileOfTaskPlan.GetClient();
File clientFolder = clientOfTaskPlan.getDocumentsFolder();

I guess it comes down to personal opinion on this matter.

Answer 7

It is not bad as such, but you might get problems reading this in 6 month. Or a co-worker might get problems maintaining / writing code, because the chain of your objects is quite... long.

And I recon that you do not have to introduce variables in your code. So the objects do know all they need to jump from method to method. (Here arises the question if you did not overengineer a little bit, but who am I to tell?)

I would introduce a kind of "convenience methods". Imagine you got a method in your "task" - object something like

    task.getClientFromActionPlan();

You then surely could use

    task.getClientFromActionPlan().getDocumentsFolder();

Much more readable and in case you do these "convenience methods" right (i.e. for heavy used object chains), much less to type ;-) .

Edith says: These convenience methods I suggest often do contain Nullpointer-checking when I write them. This way you even can throw Nullpointers with good error messages in (i.e. "ActionPlan was null while trying to retrieve the client from it").

Answer 8

Read up on The Law of Demeter.

Answer 9

Related discussion:

Function Chaining - How many is too many?

Answer 10

This question is very close to http://stackoverflow.com/questions/154864/function-chaining-how-many-is-too-many#155407

In general, it seems that people agree that too long chains are not good and you should stick to one or two chained calls at most.

Though I hear that Python fans consider chaining to be a lot of fun. That might be just a rumor...:-)

Answer 11

First of all, stacking code like that can make it annoying to analyze NullPointerExceptions and check references while stepping in a debugger.

Apart from that, I think it all boils down to this: Does the caller need to have all that knowledge?

Perhaps its functionality could be made more generic; the File could then be passed as a parameter instead. Or, perhaps the ActionPlan should not even reveal that its implementation is based on a ClientFile?

Answer 12

Depending on your end goal you would probably want to use The Principal of Least Knowledge to avoid heavy coupling and costing you in the end. As head first likes to put it.. "Only talk to your friends."

Answer 13

I agree with the poster that mentioned the Law of Demeter. What you're doing is creating unnecessary dependencies on the implementations of a lot of these classes, and on the structure of the hierarchy itself. It wil make it very difficult to test your code in isolation, since you will need to initialize a dozen other objects just to get a working instance of the class you want to test.

Answer 14

Another important byproduct of chaining is performance.

It's not a big deal in most cases, but especially in a loop you can see a reasonable boost in performance by reducing indirection.

Chaining also makes it harder to estimate performance, you can't tell which of those methods may or may not do something complex.

Answer 15

the flag is red, and it says two things in bold:

• to follow the chain it is necessary for the calling code to know the entire tree structure, which is not good encapsulation, and
• if the hierarchy ever changes you will have a lot of code to edit

and one thing in parentheses:

• use a property, i.e. task.ActionPlan instead of task.getActionPlan()

a better solution might be - assuming you need to expose all of the parent properties up the tree at the child level - to go ahead and implement direct properties on the children, i.e.

File clientFolder = task.DocumentsFolder;

this will at least hide the tree structure from the calling code. Internally the properties may look like:

class Task {
    public File DocumentsFolder {
        get { return ActionPlan.DocumentsFolder; }
    }
    ...
}
class ActionPlan {
    public File DocumentsFolder {
        get { return ClientFile.DocumentsFolder: }
    }
    ...
}
class ClientFile {
    public File DocumentsFolder {
        get { return Client.DocumentsFolder; }
    }
    ...
}
class Client {
    public File DocumentsFolder {
        get { return ...; } //whatever it really is
    }
    ...
}

but if the tree structure changes in the future you will only need to change the accessor functions in the classes involved in the tree, and not every place where you called up the chain.

[plus it will be easier to trap and report nulls properly in the property functions, which was omitted from the example above]

Answer 16

How timely. I am going to write a post on my blog tonight about this smell, Message Chains, versus its inverse, Middle Man.

Anyhow, a deeper question is why you have "get" methods on what appears to be a domain object. If you closely follow the contours of the problem, you will either find out that it doesn't make sense to tell a task to get something, or that what you are doing is really a non-business logic concern like preparing for UI display, persistence, object reconstruction, etc.

In the latter case, then the "get" methods are ok as long as they're used by authorized classes. How you enforce that policy is platform -and process-dependent.

So in the case where the "get" methods are deemed ok, you still have to face the problem. And unfortunately, I think it depends on the class that is navigating the chain. If it is appropriate for that class to be coupled to the structure (say, a factory), then let it be. Otherwise, you should try to Hide Delegate.

Edit: click here for my post.

Answer 17

Getters and setters are evil. Generally, avoid getting an object to do something with it. Instead delegate the task itself.

Instead of

Object a = b.getA();
doSomething(a);

do

b.doSomething();

As with all design principles, do not follow this blindly. I have never been able to write anything remotely complicated without getters and setters, but it is a nice guideline. If you have a lot of getters and setters, it probably means you are doing it wrong.

Answer 18

I'd point to The Law of Demeter, too.

And add an article about Tell, Don't Ask

Answer 19

Are you realistically going to ever use each and every one of those functions independently? Why not just make task have a GetDocumentsFolder() method that does all the dirty work of calling all those methods for you? Then you can make that do all the dirty work of null-checking everything without crufting up your code in places where it doesn't need to be crufted up.

Answer 20

OK as others point out the code isn't great because you're locking in the code to a specific hierarchy. It can present problems debugging, and it's not nice to read, but the major point is the code that takes a task knows way too much about traversing to get some folder thing. Dollars to donuts, somebody's going to want to insert something in the middle. (all tasks are in a task list, etc)

Going out on a limb, are all of these classes just special names for the same thing? ie are they hierarchical, but each level has maybe a few extra properties?

So, from a different angle, I'm going to simplify to an enum and an interface, where the child classes delegate up the chain if they aren't the requested thing. For the sake of argument, I'm calling them folders.

enum FolderType { ActionPlan, ClientFile, Client, etc }

interface IFolder
{
    IFolder FindTypeViaParent( FolderType folderType )
}

and each class that implements IFolder probably just does

IFolder FindTypeViaParent( FolderType folderType )
{
    if( myFolderType == folderType )
        return this;

    if( parent == null )
        return null;

    IFolder parentIFolder = (IFolder)parent;
    return parentIFolder.FindTypeViaParent(folderType)
}

A variation is to make the IFolder interface:

interface IFolder
{
    FolderType FolderType { get; }
    IFolder Parent { get; }
}

This allows you to externalize the traversal code. However this takes control away from the classes (maybe they have multiple parents) and exposes implementation. Good and bad.

[ramblings]

At a glance this appears to be a pretty expensive hierarchy to set up. Do I need to instantiate top-down every time? i.e. if something just needs a task, do you have to instantiate everything bottom-up to ensure all those back-pointers work? Even if it's lazy-load, do I need to walk up the hierarchy just to get the root?

Then again, is the hierarchy really a part of object identity? If it's not, perhaps you could externalize the hierarchy as an n-ary tree.

As a side-note, you may want to consider the DDD (Domain Driven Design) concept of aggregate and determine who the major players are. What is the ultimate owner object that is responsible? e.g. wheels of a car. In a design that models a car, the wheels are also objects, but they are owned by the car.

Maybe it works for you, maybe it doesn't. Like I said, this is just a shot in the dark.

Answer 21

On the other hand, the Law of Demeter isn't universally applicable, nor a hard rule (arrr, it be more of a guideline!).