It has been a while since I last wrote. It is not that I wrote enough, I still love it, and am keen to share my (possibly biased) thoughts with fellow developers out there. But life and work have kept me busy in the past few months, busier than I would like to be.

Anyhow, I am starting a series of posts called “Root of All Evil”. The plan is to write about bad practices I see at work, on the Internet, and smelly code I used to write. I hope to be able to cover the following aspects:

  • What does bad code look like
  • Why is it bad
  • And how it could be improved/refactored

If you happen to care reading my blog, chances are you won’t be contributing too many of those. However, even if they were someone else’s responsibility, it is essential that you I encourage you to step up, and talk him/her into making necessary changes and quitting the habit, perhaps over a cup of coffee or via a formal code review.

Though it is unlikely that I would have the luxury and the time to write weekly as I used to, I will just do my best. This series will be written casually, in Chinese or in English, yes, depending on my mood. So, bear with me and here it goes.

Nasty If Else blocks are killers to code readability, and if I may, a huge pain in the ass for sorry developers who ended up maintaining them. They waste your valuable time and energy, deciphering logics that would had otherwise been trivial to understand.

Not so sure? Take the following random code snip taken from the Internet for example:

    void remove(TreapNode *&t, KeyType v)
    {
        if (t == NULL) return;
        if (v > t->key) remove(t->right, v);
        else if (v < t->key) remove(t->left, v);
        else
        {
            if(t->left == NULL && t->right == NULL)
            {
                delete t;
                t = NULL;
            }
            else if(t->left == NULL)
            {
                TreapNode *tmp = t;
                t = t->right;
                delete tmp;
            }
            else if(t->right == NULL)
            {
                TreapNode *tmp = t;
                t = t->left;
                delete tmp;
            }
            else
            {
                if (t->left->priority < t->right->priority)
                {
                    RotateLeft(t);
                    remove(t->left, v);
                }
                else
                {
                    RotateRight(t);
                    remove(t->right, v);
                }
            }
        }
    }
    Note: Please don’t get into what it does, and quite frankly, I don’t care. Not for the purpose of this post anyway. I personally find it fascinating that, oftentimes, one could improve a piece of code without understanding what it actually does.

Can you tell what goes after the recursive remove() if the condition in line 4 was true?

Of course you can. Nothing, it just returns.

But how long did it take you to find that out? Was it easy to you to skim through the entire function and at the same time, keeping track of levels of if/else blocks?

Only if it had been written differently, possibly in the line of the following:

    void remove(TreapNode *&t, KeyType v)
    {
        if (t == NULL)
          return;

        if (v > t->key)
        {
          remove(t->right, v);
          return;
        }

        if (v < t->key)
        {
          remove(t->left, v);
          return;
        }

        // snipped...
    }

It is now obvious. Any one could tell in 3 seconds. And better yet, without reading the remaining of the function.

Dealing with one such badly written function may be trivial, but digging through 10, 20 of these in a large code base may become stressful as complexity adds up.

To avoid nasty if/else blocks and improve readability, return as early as possible and use Extract Method when appropriate.

Trust me, maintainers of your code will appreciate it.

« »