Keep your code dumb

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--" }

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;
    }
  }
}

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()
  {
    // ...
  }
}

... 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);

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!

Comments

[edA‑qa mort‑ora‑y]

I agree code shouldn't be clever, but lazy-init isn't necessarily bad, just how it was implemented here.

I use lazy initialization a lot in my current project. It never invokes genuine work though, like loading from a DB. It just creates a few new instances. The observable effect is indistinguishable from the value having always been there.

We also do lazy loading, but in a different context. And it's clearly a lazy "load", not an "init", so it's much clearer what's happening.

[JavaScript Joel]

but lazy-init isn't necessarily bad, just how it was implemented here.

Totally agree. C# also has Lazy<>, what is important about C#'s Lazy<> is that it is labeled Lazy.

The main problem with this code example is that there is code that "performs work" behind a property. And properties should never "do work". That is what a function is for.

[edA‑qa mort‑ora‑y]

I understand the sentiment behind "performs work", but I'm not sure it's a fixed rule. Let me give you a counter example.

At Fuse we have a UX syntax for declaring a UI, here's a really tiny made-up snippet.

<PageControl IsEnabled="false" Transition="none">
   <PurchasePage/>
   <ProfilePage/>
</PageControl>

These correlate directly to types and instances in the backing Uno/C# code. For example:

var pg = new PageControl();
pg.IsEnabled = false;
pg.Transition = TransitionType.None;

When the app starts up it constructs a UI from these obejcts -- they are live objects, forming part of the UI tree.

Now what happens if I do pg.IsEnabled = true. This is a rather complex property. In order for the control to be enabled it needs to have a few attached gestures, which install pointer handlers, and a few other things. As a programmer using our API you don't think about this much, you just expect the control to be enabled now.

It appears that in the domain of declarative programming the rule about accessors not doing work may not be valid.

[JavaScript Joel]

In this instance, I would still prefer to use something like pg.setEnabled(true).

If obj.prop was a getter that hid some logic behind it, then in the case of code like this:

// Properties get used like variables, which could put you in hot water
// if there is complex code behind the getter.
doSomething1(obj.prop);
doSomething2(obj.prop);
doSomething3(obj.prop);
Console.WriteLine(obj.prop);

We would be executing that property logic multiple times. We do not have any idea of how heavy the property is without opening up the underlying code. If each get hid a database or api call, we could quickly get into trouble.

In this instance, it would be more clear that the underlying code is doing something more than just retrieving a value...

doSomething1(obj.getProp());
doSomething2(obj.getProp());
doSomething3(obj.getProp());
Console.WriteLine(obj.getProp());

... and that the code should be written like this:

var prop = obj.getProp();
doSomething1(prop);
doSomething2(prop);
doSomething3(prop);
Console.WriteLine(prop);

I prefer this because it gives me visibility about the complexity of the code without having to dig into it. It also gives me more control over when the program performs this work.

Of course there is no set rule and situations vary and this is my personal preference :)

Cheers!

[edA‑qa mort‑ora‑y]

I would argue that obj.set_enabled(true) and obj.enabled = true are semantically the same. It would be a break in the programmer's expectations if this weren't true. set/get functions are just making up for a lack of accessors/getters in a language.

Setting enabled in my scenario doesn't do something everytime. I'd never argue for that. You can safely set it repeatedly to the same value and nothing changes, only if the value changes. In declarative programming, and reactive programming though, there are a lot of dependent variables. By changing one you can set-off a cascade of modifications.

[JavaScript Joel]

I am opposed to getters and setters entering into JavaScript. I don't like them in C# either.

I would argue that obj.set_enabled(true) and obj.enabled = true are semantically the same.

I still have to disagree on this for the reason that I have been bitten multiple times by badly designed software that hid logic inside a getter. This was not known to me until I experienced performance issues which required me to dig into the library I was using only to realize this was not a property value that I was expecting, but a full blown function disguised as a getter. If this had instead been a function, I would have expected some type of logic to be inside.

[edA‑qa mort‑ora‑y]

Because some people misuse a feature is a bad reason to dislike it. An idiot can just as easily write a function called get_name() that deletes your hard drive.

[JavaScript Joel]

Valid point.

I have been stung by this before, so I definitely hold a bias. I also prefer functional designs over OOP, so there is a lot of OOP that I also dislike.

[janjuks]

I would argue that obj.set_enabled(true) and obj.enabled = true are semantically the same.

I agree with this, but even more that setters/getters shouldn't be doing any work. I think that Joel didn't name it very good. That 'set' in method name is the part which makes them semantically the same. Maybe it's just me, but naming it obj.enable() would imply that there might be some work to be done and not just setting a value.

That being said, I have written that code myself where private variable would hold some raw data and getter would decode it only once when first needed. This post + just recently read Clean Code by Robert C. Martin reminded me of this ^_^

[JavaScript Joel]

naming it obj.enable() would imply that there might be some work to be done and not just setting a value.

^ This :)

[Steve Steiner]

Being the "old guy" on most projects, often I run into a piece of code written in the latest "clever" style. If I can't just read through it and explain to myself what it does, I see if I can get the person who wrote, or at least someone who thinks they can understand it, it to explain to me, in simple terms, exactly what it does.

I open a code editor, and write their explanation down, in code. Very simple code.

By the end, I understand what it does, they understand what it does, the version we've just written will usually serve as a drop-in replacement and, most importantly, they usually "get" why I write code that's so simple and not clever at all.

And, for major bonus points, when the bug is hidden in (dare I say "created by"?) the clever, it pops right out and gets solved in the course of the "simple" rewrite.

[JavaScript Joel]

Totally agree!

If you have to break open up a function (or any code) to understand what it does, there is something wrong with the function.

[Sergio Soares]

Great method, i will try practice as soon as possible. Thanks

[Erik Skoglund]

I had the exact same scenario as you. C# Lazy loaded props that read from db, serialization for redis = bad.

But I think that specific problem has more to do about what you are trying to cache. Ideally you would just cache a poco-model instead and use interfaces for your models. Then you don't need to worry about that at all.

That's what I did in my case, mostly because of constraints on changing the lazy-loading.

That said; lazy loading can cause all kinds of unexpected weird problems.

[JavaScript Joel]

The problem is so deep in the bones of the software, the company decided to just abandon the approach all together due to cost of repair.

[Weston Wedding]

I ported some logic from the GSAP animation library to C++ for use in an Arduino library that imitated the GSAP's "Tween" and "Timeline" objects in order to make animating some LEDs easier.

GSAP has some lazy initialization logic inside some of their functions and I went along with it because, hey, why not? Lazy initialization is clever and I like clever!

Unfortunately it resulted in unpredictable crashes that sent me on hours of wild goose-chasing as I went from one potential cause to another.

Eventually I realized it was the lazy initializing hiding my "real" memory usage and I was trying to allocate more memory than I had in ways that depended entirely on what I was trying to do with my library in any given program.

This would've been easy on a real computer, but you don't get stack traces or error messages in Arduino to debug with. Things just stop. Crashes would happen slower or quicker depending on how many debug messages I was using because those take up memory, too.

[JavaScript Joel]

Sounds very similar to our experience. I find those hidden kind of bugs waste the most time.

[Costin Manda]

I see the problem, but I don't think it's where you think it is. What you are saying is that the original code was done via lazy getters. What does that mean? It means that they started with normal properties (at least in their head) and they replaced a lot of getters with lazy instantiation. But if the getters are so similar, then what they did is basically copy paste code. I believe that is the problem.

If they would have built a lazy instantiation module, then changing the way it behaves would have been trivial, with a single point of change. I also think that is the way to refactor the code: replace all getters with a module that handles lazy instantiation and then make the single change to initialize the values at app startup when it is all working as before. Alternately, and ugly, you could go through all the objects you instantiate and call all their getters with reflection. You do use some framework to initialize your objects, right?

[Lluís Josep Martínez]

It looks a lot like the lazy loading en.wikipedia.org/wiki/Lazy_loading
I use it all the time in Java and works fine. Hibernate for instance uses it at its full extent. It's clever and useful.

[JavaScript Joel]

While this article uses a specific instance of lazy loading getters as an example for clever code, the article was more a commentary on clever code and not as much on lazy props.

The message I am trying to deliver here is: Clever code will yield equally clever bugs. If you keep your code dumb your bugs will be dumb as well. Dumb bugs are easier to find and squash.

[Johannes Vollmer]

Nice. :) I was a little disappointed when I found out that Cats were just Contacts though. :c

[JavaScript Joel]

Your cats aren't contacts? :D

Code updated.

Cheers!

[Moe]

Amen!