Keep your code dumb

JavaScript Joel - Nov 15 '17 - - Dev Community

I recently stumbled across this tweet...

The proposed solution was to use a getter to populate the value at the time you need it, something similar to this:

function getUniqueId() {
  // pretend this is some expensive computation
  return '--id--'
}

function MyObject () {
}

// lazy getter for 'id'
Object.defineProperty(MyObject.prototype, 'id', {
  get: function() {
    const value = getUniqueId()
    Object.defineProperty(this, 'id', { value, enumerable: true })
    return value
  }
})

const obj = new MyObject()
console.log(obj) // > {}
console.log(obj.id) // > "--id--"
console.log(obj) // > { id: "--id--" }
Enter fullscreen mode Exit fullscreen mode

Upon initial glance, this code looks very clever. It may work well today, but bugs with code like this will be difficult to track down later or even worse catastrophic to a codebase.

I maintain a large legacy C# project that currently has lazy getters. This code was written many years ago by the ghosts of programmers past and at the time it was written was very clever.

Something similar to this:

// BAD: clever code
public class House
{
  private _cats Cat[];

  public int Id { get; set; }

  // Lazy property that "does work". "work" should be in a function, not prop.
  public Cat[] Cats
  {
    get
    {
       if (_cats == null)
       {
         _cats = db.GetCats(Id);
       }

       return _cats;
    }
  }
}

Enter fullscreen mode Exit fullscreen mode

Today this clever code is biting us in the ass.

There was a recent project to use Redis for caching. It was realized (after launching to production) that every lazy value is now enumerated during the serialization process for caching. It ended up causing such a massive spike in CPU on production that the deployment had to be rolled back.

Because multiple things were in that deployment, it took the teams a while to determine it was the new Redis code that was causing the CPU spike and even longer time to determine why the Redis code was causing a CPU spike.

Had we used dumber code like ...

// GOOD: dumb code
public class House
{
  public Cat[] GetCats()
  {
    // ...
  }
}

Enter fullscreen mode Exit fullscreen mode

... we would never have run into this problem.

The codebase is now over a million lines and the fix for a problem that should have never existed is monumental. It is still unfixed to this day.

This isn't the first time this same clever code has bit us in the ass. There are places in the code I will see things like this:

// BAD: In this code, `house.Cats` has to be touched
//      in order for the property to be populated.

var house = db.GetHouse(id);

// WTF?
house.Cats;

return DoSomething(house);
Enter fullscreen mode Exit fullscreen mode

This type of code requires me to dig into the implementation of the property id to understand it's usage. This code smell is so bad it makes me wretch.

Too many times I have seen clever code come back and haunt the codebase.

Clever code will leave you to hunt down and solve equally clever bugs. Keep your programs dumb and your code and your bugs will be easier to find.

I would love to hear stories on how have you been bitten by clever code in the comments below!

Follow me on Twitter @joelnet or LinkedIn

Cheers!

. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .