Is there anything inherently wrong with long object invocation chains? [closed]

Question

I've organized my code hierarchically and I find myself crawling up the tree using code like the following.

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

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

Is this wrong?

Answer 1

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 2

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.