Long delegation chains in C++

Question

This is definitely subjective, but I'd like to try to avoid it becoming argumentative. I think it could be an interesting question if people treat it appropriately.

In my several recent projects I used to implement architectures where long delegation chains are a common thing.

Dual delegation chains can be encountered very often:

bool Exists = Env->FileSystem->FileExists( "foo.txt" );

And triple delegation is not rare at all:

Env->Renderer->GetCanvas()->TextStr( ... );

Delegation chains of higher order exist but are really scarce.

In above mentioned examples no NULL run-time checks are performed since the objects used are always there and are vital to the functioning of the program and explicitly constructed when execution starts. Basically I used to split a delegation chain in these cases:

1) I reuse the object obtained through a delegation chain:

{ // make C invisible to the parent scope
   clCanvas* C = Env->Renderer->GetCanvas();
   C->TextStr( ... );
   C->TextStr( ... );
   C->TextStr( ... );
}

2) An intermediate object somewhere in the middle of the delegation chain should be checked for NULL before usage. Eg.

clCanvas* C = Env->Renderer->GetCanvas();

if ( C ) C->TextStr( ... );

I used to fight the case (2) by providing proxy objects so that a method can be invoked on non-NULL object leading to an empty result.

My questions are:

1. Is either of cases (1) or (2) a pattern or an antipattern?
2. Is there a better way to deal with long delegation chains in C++?

Here are some pros and cons I considered while making my choice:

Pros:

• it is very descriptive: it is clear out of 1 line of code where did the object came from
• long delegation chains look nice

Cons:

• interactive debugging is labored since it is hard to inspect more than one temporary object in the delegation chain

I would like to know other pros and cons of the long delegation chains. Please, present your reasoning and vote based on how well-argued opinion is and not how well you agree with it.

Baryta

Tough choice, though I personally usually lean toward easy debugging. – Trawler

@Almo: Please, speak your mind! – Baryta

I don't agree that "long delegation chains look nice". When I read Env->Renderer->GetCanvas()->TextStr(), I need to think about many things at once: what is the environment object, what is the renderer object, what is the canvas and what does the TextStr method do. Moreover, this means that the code is thightly coupled to all these classes, while it could perhaps be better encapsulated (does the code that read the text from the canvas really need access the entire environment?). See the law of Demeter for more information. – Bunin

Regarding your use of proxy objects, it looks a lot like the null object pattern. – Bunin

Answer 1

I wouldn't go so far to call either an anti-pattern. However, the first has the disadvantage that your variable C is visible even after it's logically relevant (too gratuitous scoping).

You can get around this by using this syntax:

if (clCanvas* C = Env->Renderer->GetCanvas()) {
  C->TextStr( ... );
  /* some more things with C */
}

This is allowed in C++ (while it's not in C) and allows you to keep proper scope (C is scoped as if it were inside the conditional's block) and check for NULL.

Asserting that something is not NULL is by all means better than getting killed by a SegFault. So I wouldn't recommend simply skipping these checks, unless you're a 100% sure that that pointer can never ever be NULL.

---

Additionally, you could encapsulate your checks in an extra free function, if you feel particularly dandy:

template <typename T>
T notNULL(T value) {
  assert(value);
  return value;
}

// e.g.
notNULL(notNULL(Env)->Renderer->GetCanvas())->TextStr();

Rento

Invisibility to the parent scope is a good point. I have updated the code sample in question accordingly. – Baryta

What notNULL should do in case if its argument is NULL? – Baryta

@SergeyK.: Well, you could think of several alternatives. Here I simply assert that it's not-NULL. You could just as well throw an exception (if you like that sort of thing) or return a dummy object (which I would advise against). If you use the assertion, it's a good way to explicitly check what you implicitly assume: That Env, Env->Renderer and so forth are not-NULL. – Rento

I upvoted for the first part of the answer (using an if-scope); I would recommend against using the notNULL function as it will lead to useless assert messages that are harder to track down than a segfault. If you know how to use a debugger and perhaps a core dump, tracking down a NULL pointer dereference is very easy. Finding out which call to notNULL went wrong is also not difficult, but not any easier than debugging the segfault. – Hamiltonian

@wolfgang: Yes, you're partially right -- the assert is a bit problematic for the reasons you name. But think about the call to GetCanvas. It is possible that it still works even if Renderer is NULL, because it may not require the this pointer. This may lead to an undiscovered bug, waiting to pop in and punch you in the face in production. That's why I always prefer assertion failures over SegFaults. – Rento

Answer 2

In my experience, chains like that often contains getters that are less than trivial, leading to inefficiencies. I think that (1) is a reasonable approach. Using proxy objects seems like an overkill. I would rather see a crash on a NULL pointer rather than using a proxy objects.

Palazzo

It can be a latent crash somewhere in the middle of mission-critical code. – Baryta

@SergeyK.: Then your test coverage is inadequate to the mission at hand. – Palazzo

Sorry for so small samples, really. But not everything can be squeezed enough to be put into a question. – Baryta

Your idea is clear and it would be great if you could provide more reasoning about it. Please, go ahead, speak your mind. – Baryta

Answer 3

Such long chain of delegation should not happens if you follow the Law of Demeter. I've often argued with some of its proponents that they where holding themselves to it too conscientiously, but if you come to the point to wonder how best to handle long delegation chains, you should probably be a little more compliant with its recommendations.

Tilley

This would be my biggest concern. It boils down to the question is what interfaces should the current class know. In the case of fluent interfaces you could easily have long delegate chains without violating Demeters Law. – Hedgerow

Answer 4

Interesting question, I think this is open to interpretation, but:

My Two Cents

Design patterns are just reusable solutions to common problems which are generic enough to be widely applied in object oriented (usually) programming. Many common patterns will start you out with interfaces, inheritance chains, and/or containment relationships that will result in you using chaining to call things to some extent. The patterns are not trying to solve a programming issue like this though - chaining is just a side effect of them solving the functional problems at hand. So, I wouldn't really consider it a pattern.

Equally, anti-patterns are approaches that (in my mind) counter-act the purpose of design patterns. For example, design patterns are all about structure and the adaptability of your code. People consider a singleton an anti-pattern because it (often, not always) results in spider-web like code due to the fact that it inherently creates a global, and when you have many, your design deteriorates fast.

So, again, your chaining problem doesn't necessarily indicate good or bad design - it's not related to the functional objectives of patterns or the drawbacks of anti-patterns. Some designs just have a lot of nested objects even when designed well.

---

What to do about it:

Long delegation chains can definitely be a pain in the butt after a while, and as long as your design dictates that the pointers in those chains won't be reassigned, I think saving a temporary pointer to the point in the chain you're interested in is completely fine (function scope or less preferably).

Personally though, I'm against saving a permanent pointer to a part of the chain as a class member as I've seen that end up in people having 30 pointers to sub objects permanently stored, and you lose all conception of how the objects are laid out in the pattern or architecture you're working with.

One other thought - I'm not sure if I like this or not, but I've seen some people create a private (for your sanity) function that navigates the chain so you can recall that and not deal with issues about whether or not your pointer changes under the covers, or whether or not you have nulls. It can be nice to wrap all that logic up once, put a nice comment at the top of the function stating which part of the chain it gets the pointer from, and then just use the function result directly in your code instead of using your delegation chain each time.

Performance

My last note would be that this wrap-in-function approach as well as your delegation chain approach both suffer from performance drawbacks. Saving a temporary pointer lets you avoid the extra two dereferences potentially many times if you're using these objects in a loop. Equally, storing the pointer from the function call will avoid the over head of an extra function call every loop cycle.

Rios

Btw, In C++11 constexpr can help the compiler to optimize purely delegating calls. – Baryta

Cool, I'll have to look into that, hadn't heard of it before. I've been stuck reading up on the C# world for most of the last year so I haven't gotten to go through too much C++11 stuff yet. – Rios

Answer 5

For bool Exists = Env->FileSystem->FileExists( "foo.txt" ); I'd rather go for an even more detailed breakdown of your chain, so in my ideal world, there are the following lines of code:

Environment* env = GetEnv();
FileSystem* fs = env->FileSystem;
bool exists = fs->FileExists( "foo.txt" );

and why? Some reasons:

1. readability: my attention gets lost till I have to read to the end of the line in case of bool Exists = Env->FileSystem->FileExists( "foo.txt" ); It's just too long for me.
2. validity: regardles that you mentioned the objects are, if your company tomorrow hires a new programmer and he starts writing code, the day after tomorrow the objects might not be there. These long lines are pretty unfriendly, new people might get scared of them and will do something interesting such as optimising them... which will take more experienced programmer extra time to fix.
3. debugging: if by any chance (and after you have hired the new programmer) the application throws a segmentation fault in the long list of chain it is pretty difficult to find out which object was the guilty one. The more detailed the breakdown the more easier to find the location of the bug.
4. speed: if you need to do lots of calls for getting the same chain elements, it might be faster to "pull out" a local variable from the chain instead of calling a "proper" getter function for it. I don't know if your code is production or not, but it seems to miss the "proper" getter function, instead it seems to use only the attribute.

Netty

Answer 6

Long delegation chains are a bit of a design smell to me.

What a delegation chain tells me is that one piece of code has deep access to an unrelated piece of code, which makes me think of high coupling, which goes against the SOLID design principles.

The main problem I have with this is maintainability. If you're reaching two levels deep, that is two independent pieces of code that could evolve on their own and break under you. This quickly compounds when you have functions inside the chain, because they can contain chains of their own - for example, Renderer->GetCanvas() could be choosing the canvas based on information from another hierarchy of objects and it is difficult to enforce a code path that does not end up reaching deep into objects over the life time of the code base.

The better way would be to create an architecture that obeyed the SOLID principles and used techniques like Dependency Injection and Inversion Of Control to guarantee your objects always have access to what they need to perform their duties. Such an approach also lends itself well to automated and unit testing.

Just my 2 cents.

Vereen

In our case Renderer is a service locator, and Canvas is instantiated via dependency injection. – Baryta

Answer 7

If it is possible I would use references instead of pointers. So delegates are guaranteed to return valid objects or throw exception.

clCanvas & C = Env.Renderer().GetCanvas();

For objects which can not exist i will provide additional methods such as has, is, etc.

if ( Env.HasRenderer() ) clCanvas* C = Env.Renderer().GetCanvas();

Onset

All objects are dynamically allocated. Exceptions are disabled (code should run on mobile platforms) – Baryta

Answer 8

If you can guarantee that all the objects exist, I don't really see a problem in what you're doing. As others have mentioned, even if you think that NULL will never happen, it may just happen anyway.

This being said, I see that you use bare pointers everywhere. What I would suggest is that you start using smart pointers instead. When you use the -> operator, a smart pointer will usually throw if the pointer is NULL. So you avoid a SegFault. Not only that, if you use smart pointers, you can keep copies and the objects don't just disappear under your feet. You have to explicitly reset each smart pointer before the pointer goes to NULL.

This being said, it wouldn't prevent the -> operator from throwing once in a while.

Otherwise I would rather use the approach proposed by AProgrammer. If object A needs a pointer to object C pointed by object B, then the work that object A is doing is probably something that object B should actually be doing. So A can guarantee that it has a pointer to B at all time (because it holds a shared pointer to B and thus it cannot go NULL) and thus it can always call a function on B to do action Z on object C. In function Z, B knows whether it always has a pointer to C or not. That's part of its B's implementation.

Note that with C++11 you have std::smart_ptr<>, so use it!

Belia

Yes we use intrusive smartpointers in some places where shared objects don't have a particular owner and/or we need to pass the ownership between threads. However Renderer and Canvas are persistent (like everything in Env) and are created and destroyed explicitly. – Baryta

Intrusive pointers are good too. Actually smart_ptr<> have a few drawbacks... Now I have had problems with renderers where a pointer is left in a message that gets executed after the a renderer object was deleted. Smart pointers can resolve those issues easily though. – Belia