Cracking the code review (Part 2): Make them seem small – Andy G's Blog

(This is the second part in a series about working effectively to pass code review quickly and without missed defects.)

In part one, we showed that, all other factors considered, the shorter the code review, the better. It may seem intuitive that this is the case. After all, barring some entry into a code obfuscation competition, even the worst written code can still be understood if it’s short.

But you’ve probably also seen code that was easy to understand despite it being many lines long.

Examples of this phenomenon include: a class rename that spans 100 files; a host of new Factory classes; addressing all instances of a specific compiler warning; or new functionality in a system that you originally authored.

These things seem small even though they aren’t actually. So, how do we really quantify "small"?

Quantifying "small"

When you are reviewing changes in an area of the code you are an expert in (because you wrote most of it), you don’t have to think very hard to understand the changes. This notion of "how easy is the code to understand?" is the key to quantifying "small".

Your reviewers’ number one challenge is to understand what you’re doing and why [1], and the way to help someone understand is to manage their cognitive load. When someone is overburdened with cognitive load, their ability to perform is hindered in much the same way as a computer’s is when it runs a process that pushes the CPUs to their limits and uses more memory than is available in RAM.

There are three types of cognitive load we must manage [2]:

1. Intrinsic cognitive load: the inherent difficulty of the subject matter
2. Extraneous cognitive load: how well the material is presented
3. Germane cognitive load: how efficient someone’s mental models are

> "Great, I’ll just keep those low and we’re good, right?"

Almost. Our hands are a little tied here. We can only control two types of cognitive load — extraneous and germane.

What’s more, the one thing that everyone in the field of cognitive load theory seems to agree on is that the amount of cognitive load any one task incurs is unequivocally (frustratingly) specific to an individual.

What you can rely on

Unfortunately I don’t have good news for you. Reviewers are human, and humans are simple creatures.

We can only hold about 4 "things" in our short-term memory (you may have heard 7, but that was debunked) [3]. What counts as one "thing" is subjective, but you probably don’t want to have 5 new classes in a code review.

It gets worse. Our attention span degrades linearly after only 10 minutes (until some individual threshold is reached). This correlates with a decrease in performance [4].

When code reviewers aren’t paying attention, they make mistakes. When reviewers make mistakes, they miss bugs, and bugs are expensive.

Therefore, your code should be reviewable in about 10 minutes to maximize reviewer performance (in terms of missed defects).

Maximize reviewer performance by minimizing extraneous cognitive load

Recall that extraneous cognitive load is how well the material is presented. I’ll never forget my algorithms professor from graduate school, Dr. Anxiao Jiang (who eventually won a teaching excellence award that same semester). He made me feel like I really understood how to write proofs. We have all seen how much better we learn when we have a great teacher (or not).

Unfortunately we are human. Expertise, innate ability, and preferred learning medium vary wildly. This is a source of endless disagreement in our lives; what one person finds to be the best way to do something, another will find completely wrong, and for the same reasons!

The field of psychology has formalized this phenomenon into something called the expert reversal effect, and it’s something you need to keep in mind when writing your code long before it’s reviewed.

The expert reversal trap

The expert reversal effect tells us that materials which are optimally suited for beginners to learn may actually hinder experts [5]. This is because materials for beginners incorporate text, charts, graphs, and images that all say the same thing. Perfect for re-iterating a concept to a beginner. Lots of noise for an expert.

Because repetitive information is not necessary for the expert, it actually increases their extraneous cognitive load, thereby decreasing their ability to understand.

Have you ever come across a grizzled veteran developer who, perhaps surprisingly, wrote pages-long functions without any sort of commenting? Or, have you ever seen a comment from a beginner along the lines of

// this function inserts an element into the list
my_list.insert(value);

Both of these developers have fallen into the expert reversal trap.

When we abstract code into smaller functions, we are repeating ourselves.

For example

If I want to write a function for finding all common integers between two arbitrary lists, I might proceed with the following pseudocode:

set_intersection implies sorted containers. merge_sort implies a divide and conquer. Finding duplicates implies iterating over both containers simultaneously. Et cetera.

This is a contrived example, but the point is that someone ultra-familiar with the algorithm might write it all in one gigantic block instead of using helper functions. Perfect for an expert. Not enough reiteration for a beginner.

I once worked with a brilliant mathematician who had made significant contributions to the company’s products through the years. All their logic was in singular functions that were hundreds, sometimes thousands of lines long. We butted heads a lot over that. I wish I knew about the expert reversal trap then. Perhaps I could have reasoned with them better.

On the other hand, I once mentored a beginner programmer who practically begged me to keep comments like // inserts an element into a list around because the comments would help them understand the codebase better. I had to decline because the audience for reading that code was more than just that one programmer.

The audience we’re writing code for is somewhere between these two.

Programming with empathy

You are not the audience you are writing code for. You don’t to spend all your time being the primary maintainer of code that you wrote two or more years ago. Your time is better spent writing new and more exciting things.

Your audience is every other developer that will need to understand this code in the future (which often includes future-you). Your code reviewers know this, and are trying to push your code in that direction.

As "Uncle Bob" Martin points out, the demographic of the average programmer today is extremely skewed towards younger, less experienced programmers (5 years or less). Unless you know better, the best strategy is to write code aimed at beginner-intermediate developers*. After all, even your expert knowledge will decay 20-30 percent after even just one year [6] (or just one week if you’ve spent 70 hours or less learning the material [7]).

When you program with empathy, you write code that you hope any of your colleagues could maintain. In other words, you decrease their extraneous cognitive load (and also your code reviewers’).

Prevent reviewers’ minds from wandering

We’ve already discussed how your reviewers’ performance will degrade after about 10 minutes, so how can you make the most out of that time?

The last thing you want is for a reviewer to read a line of your code then start daydreaming about all the ways they might have approached it differently, or if you even meant to write what you did. And some humans brains really like to wander [8].

Ambiguous code leads to high extraneous cognitive load as your reviewer expends effort attempting to understand what it does, why it does it, and if that’s what was intended.

For example, I once reviewed some code that simultaneously:

• performed a bubble sort
• removed duplicate values
• found the minimum value

After I finally figured out what it was doing, I also realized that

• it didn’t actually remove all duplicates
• it didn’t actually sort the entire array

Was that intentional?

The code didn’t use any standard library functions, was that also intentional? Did the developer try them and then deem them too slow, or perhaps there was some logic I was missing that couldn’t be replicated with standard library calls.

There were absolutely no comments and no helper functions used. It took me well over 10 minutes just to get through.*

Be explicit

Make it obvious that every change you made was intentional, and that you’ve considered alternatives. Be explicit, even if you have to (shudder) add a comment addressing something that could be controversial.

For example, consider the following ambiguous C++ code:

What if the collection is empty? And does the author know they’re doing integer division?

The following code makes preconditions and intentions clear:

Pretend I am your reviewer, and take away my "why?". Don’t wait for a review comment; proactively address areas where the design could have gone in multiple directions.

On being proactive

In this post, we talked about how we needed reviews to seem small even if they weren’t. By keeping fewer than four "things" in our reviews, writing clear code, and utilizing an adequate number of abstractions to help our coworkers understand what we’re doing, we can keep the review to about 10 minutes or less.

In the next post we’ll talk more about how being proactive can help us manage the third type of cognitive load; germane cognitive load.

References

1. Barnett, Mike, et al. Helping developers help themselves: Automatic decomposition of code review changesets. 2015 IEEE/ACM 37th IEEE International Conference on Software Engineering. Vol. 1. IEEE, 2015.
2. Sweller, John, Jeroen JG Van Merrienboer, and Fred GWC Paas. "Cognitive architecture and instructional design." Educational psychology review 10.3 (1998): 251-296.
3. Baddeley, Alan. "Working memory." Science 255.5044 (1992): 556-559.
4. Martin, Stewart. "Measuring cognitive load and cognition: metrics for technology-enhanced learning." Educational Research and Evaluation 20.7-8 (2014): 592-621.
5. Schnotz, Wolfgang. "Reanalyzing the expertise reversal effect." Instructional science 38.3 (2010): 315-323.
6. Bahrick, Harry P. "Semantic memory content in permastore: Fifty years of memory for Spanish learned in school." Journal of experimental psychology: General 113.1 (1984): 1.
7. Murre, Jaap MJ, and Joeri Dros. "Replication and analysis of Ebbinghaus’ forgetting curve." PloS one 10.7 (2015): e0120644.
8. Unsworth, Nash, Josef C. Schrock, and Randall W. Engle. "Working memory capacity and the antisaccade task: individual differences in voluntary saccade control." Journal of Experimental Psychology: Learning, Memory, and Cognition 30.6 (2004): 1302.