Calling the next method from a previous one... Why is this bad design?

Question

If createWorld() is really long and I need to split it, I can split it to createLight(), createEarth(), createPlants(), and createAnimals().

So, naturally I do:

function createLight(){
    //work 1
}
function createEarth(){
    //work 2
}
function createPlants(){
    //work 3
}
function createAnimals(){
    //work 3
}

function createWorld(){
    createLight()
    createEarth()
    createPlants()
    createAnimals()
}

But I see a lot of newer developers do what I am tentatively calling "the Stairstep antipattern". It goes something like:

function createWorld(){
    //create light
    //work1
    createEarth()
}
function createEarth(){
    //work 2
    createPlants()
}
function createPlants(){
    //work 3
    createAnimals()
}
function createAnimals(){
    //work 4
}

I am sure this is worse, but I don't think that my opinion alone can sway my colleagues. I think that if one step is createButterfly() then that function has no business calling createGrasshopper() at the end just because that's the "next step".

And even if you name it createButterflyThenCallCreateGrasshopper(), it's still bad design because you can't test the createButterfly code well, and when you have several steps it gets hard to see where the functions are called.

I call it the Stairstep antipattern because I think of it like this:

|
|createWorld(){
              |
              |createEarth(){
                            |
                            |createPlants(){
                                           |createAnimals()

Am I correct in thinking that this is a bad design? Why is this a bad design?

score 20 · Accepted Answer · edited Jun 16 '20 at 10:01

Besides the obvious fact in case #2 createEarth and createPlants do not what their name pretends what they are doing, I think the major flaw here is the violation of the single level of abstraction principle:

createEarth does some work directly, without any abstraction, and then calls createPlants, which is a separate abstraction doing some additional work. But the latter should be on the same level of abstraction as the piece of work done before. So different levels of abstraction are mixed.

Trying to fix this, one would typically first refactor createPlants from #2 like this:

function createPlants(){
    doWork3()  //work 3
    createAnimals()
}

Now is everything on the same abstraction level, so we can resolve the naming issue: doWork3 should better be renamed to createPlants, and createPlants to createLife, since it creates animals and plants:

  function createLife(){
      createPlants()  
      createAnimals()
  }

I think now its obvious that after some further refactoring steps, one automatically ends up in case #1, which follows the SLA principle: inside of createWorld, each call is on the same level of abstraction. Inside each of the other methods, (hopefully) the work done there is also on one level of abstraction.

Note that technically, case #2 will work, too. The problem starts when one has to change something, when the code has to be maintained or evolved. Case #1 creates mostly independent building blocks, which can be easier understood, tested, reused, extended, or reordered on its own. In case #2 each building block depends on another, which throws all the former advantages over board.

score 3 · Answer 2 · edited Apr 13 '17 at 12:45

I'd call it something like "excessive call nesting". The whole point of having a separate method is to break out a single logical piece of the algorithm, ideally a re-usable piece. If it is not functionally distinct or re-usable, consider not breaking it out, unless it would significantly ease readability, and break a much large piece of code spanning several pages into more maintainable components. Usually this can be done in a way such that each piece has a reasonable degree of functional independence, even if not intended to be called from more than one place.

See also When is it appropriate to make a separate function when there will only ever be a single call to said function?

score 3 · Answer 3 · answered Dec 06 '16 at 16:53

This is a bad design because it's more tightly coupled than it needs to be.

When your functions are composed into one aggregator, each of them can be used (and tested) in isolation. When CreateEarth calls CreatePlants in the chain, it removes that flexibility from your code. It forces plants to be created, which is the usual process, but what happens when the business comes along and wants an ocean world or a volcano world that don't need plants?

By separating the functions, you can better adapt to this inevitable change.

score 2 · Answer 4 · answered Dec 06 '16 at 17:06

One of the biggest issues I see is the strict ordering constraint that #2 places on building a World. What happens when I want to createAnimals() before I createPlants()? What if I decide something needs to be done in between? In the first example it's as simple as moving a single line of code. In the second example I need to rewrite at least 2 methods.

score -1 · Answer 5 · answered Dec 07 '16 at 19:34

Reasons to go way #1:

Function Call Stack optimization.
Modularity for testability.
Modularity for maintainability.

From Wikipedia on function call stack (seems to be not a big issue with nowadays computers, but still is with microcontrollers and other embedded systems):

"In computer science, a call stack is a stack data structure that stores information about the active subroutinesof a computer program".

JacquesB · Answer 6 · 2016-12-07T14:20:28.260

It's called "bad naming". A method called createPlants which actually create both plants and animals have a misleading name. Possible it indicates a misunderstanding of function abstraction, since the writer does not realize createAnimals is part of what createPlants does.

When decomposing a large function into multiple smaller functions, it can be done sequentially or hierarchically. Both are completely valid, depending on the semantics of the functions. But each function should have a clear and well-defined purpose which should be reflected in its name.

If it is hard to come up with a natural name for a function, it indicates the purpose of the function is not well-defined. If you need to name a function createAllAnimalsExceptGrashopper() to describe its purpose, the functions are not decomposed in a natural way.

But there is nothing wrong per se with calling the "next step" at the end of a function. For example if createEarth has be called before createPlants can be called, I would argue it is better design to have createEarth call createPlants rather than having them called sequentially at the top level. (Possibly the earth could be a parameter to createPlants to make the dependency explicit.)

As for the name of this "antipattern": I think it is misguided to think that any particular instance of bad design must have a specific name. I blame the education system for placing to much importance on knowing and remembering precise terminology, rather than actually understanding the underlying issues. Trying to teach people they shouldn't write "stairstep metods" without them understanding the underlying issue will just cause cargo cult programming.

Calling the next method from a previous one... Why is this bad design?

6 Answers6