Clean Code – Why is Calling the Next Method from a Previous One Bad Design?

clean codecode-quality

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?

Best Answer

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.

Related Topic