Coding as a Medium
It doesn't take a lot of time while working on code before realizing (or coming to terms with the fact) that there is no such thing as a perfect version of a system's code; names, layouts, coupling, cohesion, reuse, abstractions, artistic flares, stylistic touches, and more could all be changed and refactored indefinitely without the system gaining (or losing) much in its actual features or capabilities.
This is because coding is not really about building something at all (like designing a car engine). Instead, it's primarily about communicating with other engineers in a language which our general purpose computing thingy can still listen in on and hopefully do what we thought it would do.
There are countless ways to code the same thing because the message your are trying to convey via the code might be different depending on context, new learnings and perspectives, and who the audience reading this code is (e.g. are they part of the core team? are they hacking with you? are they interns? are they external to the team? are they the future maintainers of the system? are they future you?).
This point is important and I want to talk more about it in a later post, but it's also a good framing for the problems with defining how to do Code Review - the Code Review too is just a medium for communication, and as such there are more than one right way to do it, depending on audience, and context. let's formalize what this means by taking a look at what people would say Code Review is even about...
PS. When referring to Code Review I primarily mean the process of reviewing code and the tools/systems that an organization puts in place to perform these. For the latter, my observations are the result of using Facebook's fork of Phabricator and it's eventual reimplementation as the Diff tool. Some of these observations might not make sense if you are used to a different tool/system.
Code Review is about Quality
When I first heard about code reviews it was pitched as a quality measure. It's the easiest sell: if you are going to trust an engineer to push code to production without code review, imagine how much safer you'd feel if we double the number of engineers that take part in every code change. Also the price seems reasonable (reading code would most likely be faster than writing it, but more on that later...).
The easiest way to tell that this is the purpose of the system is in it's Approve ("Accept") and Request Changes ("Reject") feature, and specifically the fact that one cannot approve their own changes. This creates a hard dependency on having an additional person as part of the shipping process, and that alone (ignoring any other feature the CR system provides us) is enough to provide us a framework for maintaining high quality code because, at the very least, that person is now involved in why this code is part of the system.
When to Use?
The strongest this use-case is is when your team is made of a good mix of experts (by tenure, by level, by tech, or by module) and non-experts so that it's clear who does reviews, why they do it, and on what bandwidth. It allows knowledge to trickle down, it provides managers with a great tool to grow engineers, and it's usually effective at catching issues. usually...
If your team isn't balanced correctly though you'll start to run into problems.
Too many experts working on different things and it becomes hard for anyone on the team to review anyone else's code as A LOT of context needs to be frontloaded on every review. This will result in superficial reviews which don't catch the bigger picture bugs that are the result of the many well-intentioned small code changes on the way to hell.
Too many experts reviewing each other's code might also just lead to bad social experiences. Unless your team is extremely open to feedback, and there's a lot of respect and camaraderie, reviewees who see themselves equal to (or better than) their reviewer colleague might not appreciate or accept the feedback (especially when the feedback is the most critical). Just the idea of being part of CR might be stressful enough for people to actively try to avoid it, directly, or simply rubber-stamping the CRs.
Not enough experts, on the other hand, and the queues might fill up, preventing your best engineers from being able to make any progress on their own, your team being slowed down while people wait for review, or requiring the experts to cut corners. All of which is bad.
But not as bad as letting your non-experts review each other's code. It might just take them an exhaustive amount of time to do, while they are effectively learning while reviewing. But more commonly they simply don't perform any meaningful review of the code at all, skipping, missing, or misjudging the actual things they are supposed to catch.
At my time at Facebook I remember more CRs then I want to admit that I found myself either summarizing my own changes using the dogscience meme trusting that whomever is going to review my code would know better, or using it as my sole comment when approving someone else's code basing my judgement on a mere hunch.
It's clear that CR doesn't always result in a quality assuring process, and might even lead to a false sense of confidence for the quality of shipped code in your team.
What's Next?
So most likely your team is unbalanced, as most teams probably are, you most likely have open positions either at the junior end or the senior end. You see why CR might not be a beneficial for your team, and might not improve the quality of your system. So should you just drop CRs until it makes sense for your team?
Well... understanding that CR is not just about Quality is the first realization. But once you deemphasize Quality, what fills it's place? I claim that Code Review is also about Discussions, about Documentation, and about Automation. These would be the subject of the next post.
Top comments (0)