Home Archive Trilium Notes About

Review heuristic: Call out bad code

Posted on 2018-09-12

Code health tradeoffs in larger codebases

When people program, they often need to make tradeoffs between what will fix the current problem quickly, and what will make for a healthy codebase. Examples of such a tradeoff is:

I found a function which does almost what I want, but not quite. Do I wait for its maintainer to let me refactor it so I can reuse it, or do I just copy-paste? Or - if it’s just a 5-line chunk, do I extract it into a function if it will require a bunch of boilerplate (maybe it would necessiate adding a new module)?

It’s often a question of “do I pay the bigger upfront cost now, or do I make future-me front it”. These days I often work in a really big codebase, where there ends up being a bunch of those.

My review tip for copy-pasted code

When you review someone’s code that adds similar technical debt, you might not want to force the author to go and dig in 4 files just to extract out a function. It might frustrate them. And also, often it’s really not the best thing to do. If you really only duplicate the same 5-line chunk in two places, and it’s not often changed and not tricky, it might really be less costly to copy it than to agressively share such code.

So the request I often give in code reviews is: “if you duplicate code, add a TODO that the code is duplicated” (preferably to both copies).

This way, you allow the reviewer to quickly go along their day. But also you leave a “hey, this is a known bit of technical debt” affordance in the code.

When someone ends up copy-pasting the same code a third time, you will be much more likely to look and say “hey time to extract a function, there is this TODO which says it’s not the first time we’re doing this”.

General heuristic: “feel the pain”

There is a more general heuristic, which I use, that I call “feel the pain”. The heuristic is that bad things should be obviously painfully bad. Other instances of “feel the pain” are:

Even generaler heuristic: “Call out problems”

There is actually an even more general heuristic than this, which is “call out problems”. I also use it in other contexts.

“Call out problems” feels sort of close to non-violent communication. I feel much less like openly communicating problems if it feels like the environment will hurt me if I do that. That happens to me with some strong personalities, or with people who (probably mostly unknowingly) trigger my sense of “argh I’m being bullied I want to curl up in a corner”. On the other hand, I usually try to call out problems more often than people around me, because I want to nudge culture towards cooperation. (Google culture is pretty blameless, so I don’t feel I’m acting against my interests.)