Cracking the code review (Part 3): Be proactive – Andy G's Blog

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

In the first two parts of this series, we discussed why code reviews needed to be small, and failing that, how we could make them seem small by managing extraneous cognitive load. There is a third way to make reviews go faster, and that’s by proactively ensuring your reviewers know a bit about what you’re working on before they review it (managing germane cognitive load).

Germane cognitive load

Germane cognitive load refers to how efficient one’s mental models are (called schemas by psychologists) [1]. Basically, if you have no knowledge or only surface knowledge of a topic, you will understandably not perform well in tests related to that topic. By studying up for an exam, for example, you find yourself breezing through the questions.

Through training and experience one can improve their mental models spectacularly. The best part is that these mental models reside in long term memory, which is considered to be more or less infinite [2].

When reviewers are able to call upon their long-term memory during a code review, it barely costs them any cognitive effort, unlike learning something new. Hence, my first piece of advice:

Code in familiar patterns

Humans are excellent at recognizing patterns. With just a little bit of input, they can recall vast amount of related information from their (basically infinite) long term memory. Use that!

All those famous design patterns with fancy names like "Flyweight", "Facade", and "Observer" are first and foremost for readability. When you see a class named "FooFactory", you already have a few expectations of the class. Mainly that it should produce objects of type "Foo" somehow.

If your project has separate classes for data and serialization, then now is not the time to implement a brand-new serialization scheme where your class inherits from some ISerializable interface that requires it writes to an IJSON object or something (not too far from something I’ve seen).

Proactively teach your reviewers

I don’t mean send them to night school. I mean involve your reviewers in your design and implementation process.

Your reviewers’ #1 challenge is to learn what you’re doing and why [3], so teach them all that before the review even happens. Have you ever been in a situation where one of your reviewers has been working closely with you on your feature, but another hasn’t? The former was able to approve your review faster and with better comments, right?

Have a brief in-person meeting

… with one or more of your reviewers to discuss your approach and any uncertainties you may have before you begin.

Ping your reviewers along the way

… about how the implementation is going. If they attend your daily standup meeting, use that opportunity for a quick update.

Eyes and ears

Humans have a pair of CPUs to work with — an audio processing system and a visual processing system [2], so the most efficient way to communicate is to use both. As an introvert, I prefer the faceless aspect of IM and email, but as a professional I need to get over myself.

I’ve personally found that small updates can be handled through IM/email, but the initial discussion should be in-person meeting.

For large reviews, have a meeting

Sometimes a review is just going to be big and there’s no easy way around it. For example, one "small" architectural change in a low-level project may snowball upwards, eventually changing large swaths of higher level code to get it all to build. Yes, the project needs to be restructured to make this impossible. Yes, the developer should have better planned out their changes, but here we are.

We need to take it upon ourselves to show the reviewers we care about their time, and we do that by hosting large reviews as a meeting instead of a faceless pull request because:

• Being on-hand to answer "why did you…?" and "did you consider…?" questions will prevent reviewers from falling down rabbit holes (see the discussion about ambiguous code in Part 2)
• actively engaging with your reviewers will keep their minds from wandering after the magic 10-minute mark where their performance will degrade
  • just ensure you keep them entertained with a lot of ad-hoc references to your preferred sci-fi/fantasy series
• utilizing both CPUs (eyes and ears) in humans is a more efficient use of their resources

> "Ugh, I’m not scheduling another meeting"

I promise this meeting is worth it. You’ll actually get something done, and it will save you time in the end.

Begin the meeting by refreshing your reviewers on the problem you’re solving and your overall approach (even if you were proactive and involved them from the start). Then, painstakingly go through every change and why. Answer questions along the way. Don’t end the meeting until the reviewers have either agreed to approve the changes, or have given you a list of everything they’d like to see changed before they approve.

If your review is large simply because it’s the same few changes multiplied by a hundred files, I’ve found that it’s okay to spend only 15 minutes covering those changes in their variations and then encouraging (but not expecting) the reviewers to check the rest of the review to make sure you aren’t lying.

For example

I used this approach to great effect once when I raised the warning level of a large C++ codebase (higher warning level means the compiler will complain about more things, even things that aren’t technically wrong).

It turned out that some of the projects were already treating warnings as errors (a good practice), so I thought I’d just fix those real quick and we’d be done. 500 files touched later, I had a large number of changes.

Sure, I could have planned that better. I could have only enabled one new warning at a time and submitted a code review for those fixes. I could have spent another two days breaking the changes into commits that just fixed one type of warning. Ultimately there were really only a handful of different types of fixes, just applied in a lot of places. So I scheduled a one-hour meeting with the reviewers.

After showing them some of the more crafty fixes (and actual bugs discovered), the reviewers were happy they didn’t have to spend more of their personal time puzzling through why I chose a certain way to fix a class of warning.

It was such a rousing success that the other reviewers started requesting review meetings afterwards.

A time a meeting didn’t work

A developer noticed that these review meetings were really productive, so they scheduled a one hour meeting of their own… for ~3k lines of code including dozens of new classes and hundreds of tests, mixed in with refactors.

The developer had been working happily in their own branch for over a month until they felt the code was ready to review. The only problem was that a meeting is really great for changes the reviewers will already agree are good (or mostly good anyway). In this case, the developer made some controversial design decisions along the way, which bogged the entire meeting down with debate over the right design.

The developer had to spend the better part of another month redesigning and breaking the review apart, dealing with big merge conflicts along the way. All of which could have been avoided if the developer had been proactive in both nailing down the design and getting parts reviewed along the way.

The fruits of being proactive

By proactively involving your reviewers with design and implementation, you are helping them build a priori knowledge about your code. More formally, you are lowering reviewers’ germane cognitive load by endowing them with (efficient) mental models before a review has even started. This is a classic time/space optimization — by filling your reviewers’ long term memory with knowledge (space), you get faster reviews (time).

The risk of being proactive

By involving other people in your design and implementation upfront, you are opening the door for them to disagree with you. I cannot overemphasize how important it is to resolve arguments in a healthy manner, which is why I’ll spend the entire next post discussing it.

References

1. Sweller, John, Jeroen JG Van Merrienboer, and Fred GWC Paas. "Cognitive architecture and instructional design." Educational psychology review 10.3 (1998): 251-296.
2. Baddeley, Alan. "Working memory." Science 255.5044 (1992): 556-559.
3. 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.