What Is the Purpose of Code Review and Standards?
Contentious discussions about code review seem as inevitable as death and taxes. Teams that don’t do them talk about why they should. Teams that do talk about how they ought to change them. In some groups there is a tacit understanding that code reviews are essential and that only savages would abstain, while other groups who’ve been coding for decades are left scratching their heads wondering what the fuss is about.
As a low-rent amateur psychologist, I find the matter of code reviews and coding standards fascinating, because I believe it exposes many biases and pathological aspects of the behavior of developers, myself included. The problem I see with the debate over code review and coding standards is that it usually avoids these human matters, which I believe are central.
So let’s explore them.
What is a Code Review?
In the 70s, A.F. Ackerman and M.E. Fagan published a trio of papers describing a method of “Software Inspection”, in which developers rigorously analyze lines of software code in a group setting. The takeaway from this research was that the effort was well spent, but the amount of time required to perform it was prohibitively costly in many cases.
Later in the literature, the term “Modern Code Review” appears, to which most of our current notions of code review conform. The real-time group setting is out, and the asynchronous, tool-oriented process is in.
Modern Code Review has received much research attention, but disappointingly (and as you probably suspect), a lot of interesting data has emerged, but not so many firm principles. It is one of those things that we have a hunch we either like or have a hunch should be doing.
Why Might We Want to Review Code?
I’ve observed seven distinct motivations for code review policy. The motivations are dramatically different when the organization already has a code review policy in place than when a team or organization has proposed adopting one. Many of the motivations I’ve observed do not appear in the literature at all.
This one is easy. SOX or PCI or some policy that affects your company requires a documented code review policy, presumably as a form of oversight to prevent naughty code being checked in, or as a kind of buddy system to prevent accidents. The next item covers this.
As a Form of Proofreading
My wife is an editor. She is delighted to edit my papers, or articles like these. Her eyeballs are finely tuned machines, her laptop keyboard is swift and lethal to my mistakes. Yet everything she writes also meets the red pens of at least a handful of other editors. Often she mentions that she’s received articles that had sections written in a style she doesn’t love, but because everything is edited by several people, the editors have a strong notions of the objective vs. subjective. In other words, they are allowed to feel subjectively strongly that a passage is wrong, but by doing so they then assume the responsibility to propose a rewritten section. They can’t simply punt it back to the author and say “this stinks.”
When my editor returns a paper to me, it’s rewarding because the finished product is more beautiful than something I could have made on my own. In some cases there are suggestions that I overhaul a section, with a brief, friendly description of why. In some cases there are changes I elect to take out because the revised passage doesn’t sound like me, but in the end my work is vastly better after her edits.
Reviewing code is the same way, but you will doubtlessly notice that the writer/editor and programmer/reviewer relationships differ in an important way that we will return to later. (I certainly hope so. –Ed.)
I have intentionally oversimplified the relationship of code review to correctness because the topic is huge, and the literature covers it much better than I can. I have included some links to a few of the papers I’ve found interesting in the closing section. In particular, the Beller/Bacchelli/Zaidman paper
As an Opportunity for Senior Developers to Share Their Experience
Many people love to share and teach, and feel valued when they are sought out for it. Every organization where I’ve worked as a programmer has formally encouraged the development of junior staff as a part of senior staff’s career progression, and every developer I know who has made a habit of it has profited from the investment of their time.
I believe that it’s much better for developers to solicit advice before the code review, for reasons I’ll describe in the next bullet.
As a Design Review Done Way Too Late
Some of the teams on which I’ve worked had strict policies on code review but no policy whatsoever on design reviews. As such, the code reviews were often protracted dramas in which an opinionated developer on the team, displeased with the design itself, objects to the introduction of effectively finished code into a shared code base.
Design objections at this phase of a project are both inappropriate and indefensible. Even if the design really has problems, if teams are constructed in such a way that some member has the authority to reject code due to design problems, those same members must be strictly involved in the design process. If it is not practical to do so, then revise the code review policy to eliminate
To Prevent an Accidentally-Hired Idiot From Checking in Garbage
The most competitive companies tout incredibly selective hiring practices to ensure that new hires won’t require constant supervision. Many developers aim to copy those selective hiring filters at their own less-competitive companies, but quickly discover that they must either relax their standards or shoulder more of the load.
A consequence of relaxing the standards is that a hapless developer on the team will, without supervision, wind up checking in a bunch of goofy code that nobody wants to deal with later. Rather than sit with that developer, they’ll put a review procedure into place. Then, instead of discovering the problems during the beginning and middle of the project, they can bury the developer in feedback near the deadline.
Allowing yourself to become the White Knight, protecting the source code empire from cretins, is bad for your career. It becomes quickly evident to everyone (perhaps except you) that you think of some developers as idiots. Where were you when they interviewed? Such behavior also reveals that, despite your obvious capability to help a colleague develop, you’d rather wait until a vulnerable moment and pile criticism on their work.
To Provide a Forum for the Righteous Indignation of Pedants
The degree of my personal outrage over trailing whitespace in source code is directly proportional to whether or not I have Emacs’ show-trailing-whitespace feature enabled. I find trailing whitespace objectionable specifically because Emacs, in this mode, intentionally displays it that way. The reason I see so much trailing whitespace is because, unless you’ve configured your editor to show you this trailing whitespace, you’d hardly know it was there. Most pedantry falls into this category.
Most of the developers I know weren’t even aware that trailing whitespace was a thing until they were introduced to linting tools and discovered a whole new world of ways in which their work could be “wrong,” and the delight and satisfaction of making it “right” in obscure ways.
Programmers who become linters rarely go back, and they might be happy for life if they’re able to maintain solo careers, but things can get ugly when they find themselves in large scale operations where dozens or hundreds of people may work on the same code. Opening some file of code for the first time may produce waves of revulsion, as RadProEdit++DELUXE highlights the myriad ways in which the file is “wrong.” Fueled by righteous indignation, the developer may fire off a commit that results in hundreds of lines of diff for no other reason than to satisfy the demands of the tool. Other developers, sent into similar fits by the appearance of a hundred-line diff that doesn’t actually change anything, object.
Often, we are experienced enough to understand that there is virtue in consistency and efficiency, but naive enough to believe that it must be applied everywhere all the time.
To Reinforce That the Reviewer is as Smart As or Smarter Than the Reviewee
Programmers love to pose as Aristotelian ideals, perpetually rational and objective, working selflessly in model egalitarian teams where everybody shares the load and the skill. It’s a beautiful thought.
Lurking beneath this Utopian surface is the same egotism and ambition the underlies every other human pursuit. The pathology is greater among programmers because of the lengths to which they’ll go to conceal it, couching their ambition as a purely intellectual pursuit. Don’t be fooled.
I don’t believe it’s a coincidence that many of the developers who are most vocal about the need for code reviews are also the least likely to pass on any opportunity to offer unsolicited critical advice or anecdotes designed to increase their esteem in the eyes of their team. My guess is that most of the developers in this category are former victims. Many of them delight in the notion that getting through one of their reviews is like getting worked over in an alley, and will frame it as some kind of tough love.
In fact, it’s a juvenile behavior that persists because it inflates the esteem of the reviewer. Although it may have the immediate effect of solving a handful of quality issues in the reviewed code, it does nothing for the holistic goal of improving quality, and has a toxic effect on teams.
Things I have Learned From Cohabitation Which Apply to Code Reviews
Having a partner in life has taught me a lot about code reviews.
For example, my wife commits the following atrocities daily:
- Leaves the cabinet doors and drawers ajar in our shared bathroom
- Leaves bottle caps from the evening before on the kitchen counter
- Makes big greasy streaks on the dining room table when she’s wiping it down.
When I was young and foolish, my strategy for addressing these problems was straightforward: I would point out each violation to her whenever I witnessed one of them. She would usually counter by pointing out my dirty socks on the floor. Impasse was achieved quickly.
Finally I got clever and made sure I would always be beyond reproach by cleaning up all my own messes before offering my critique. When this didn’t work either, I tried to be constructive by offering little impromptu training sessions. “Why, look how the table gleams when I use this fresh towel to wipe it. Isn’t it remarkable?“ Not wildly successful, either.
What is really happening, in the examples that I chose, is that I am leaving my interpersonal show-trailing-whitespace mode on all the time. By enabling highlight-superfluous-bottlecaps but then failing to throw the bottle cap away myself, I am esteeming myself as a trainer instead of a worker. I am attempting to transform a collaborative situation into a superior-subordinate one. Like the old chestnut, disputes over trivialities in code are contentious specifically because there is so little at stake.
The objection that I hear raised immediately when I use this example to other developers is that the nature of these two relationships differ radically, and that the values we apply in one do not apply in the other. It is not surprising that this objection would be raised in the context of a discussion about dysfunctional code review methodology. I’m not going to write a lecture on emotional intelligence, but I’ll say that it’s wise to constantly consider your role in maintaining a healthy relationship with a person you may spend your days with for many years.
What is to be Done?
We’ve reviewed some of the ways in which code reviews and standards can become dysfunctional, although I think we can all agree that they still have value if we find the right strategy. Who has a good one?
When in doubt, copy success.
As I mentioned earlier, my wife is an editor. I’ve learned more about collaborative editing from watching her work than I ever did through my own work.
Editing processes vary tremendously, but here are the parts that stood out to me.
Don’t reinvent the conventions; adopt and revise where necessary.
Editors agree on a style book, they don’t invent a style book. Style guides exist for every programming language in common use. Your company, not your team, should pick one and use it.
Don’t let the style be defined by the tools; in other words, if Microsoft Word offers corrections based on some other style, either fix it or turn it off.
As ambiguities arise, reach a consensus and document them.
Don’t use every available style guide either, since nobody will be able to remember the salient details.
It takes less time to fix it than it does to talk about fixing it.
If your style guide is kept up to date, there is never a need for a reviewer to even discuss fixes that are related to style. A reviewer should fix it on the spot if it matters enough to fix.
In software, this is even simpler to do, since tools can do most of the job.
If you don’t intend to fix it, keep quiet about it.
You aren’t paid to be a literary critic. You’re paid to improve the efficiency of the team you work for. Critiques about passages that don’t warrant fixing are often confusing to the reviewee- is this something they missed in the style guide, that was too time consuming for you to edit? Is this something the reviewer has seen often enough that it might warrant updating the style guide? Does this need fixing, or not?
It’s a powerful temptation to give anecdotes or to play code golf in a code review, and to be sure there is enormous value and opportunity to build camaraderie by exchanging stories and tips about code. However, the code review is a formal step in a process that, expedited, means that work can be completed. Email is a great side-band for the informal fun part of the work.
It’s OK to leave things to judgment.
In cases where an editor is proofing copy written by a non-professional writer, or where the copy is of a specific style in which the editor is more proficient than the writer (e.g. press releases), the editor often just knows the right style when she sees it. This style can’t be codified in a manual, rather, it is learned from observation of a senior editor.
However, works of this type are usually intended for public consumption, and are thus held to a different standard, and the editor is special because she knows the audience.
If you don’t have soft skills, you shouldn’t be doing code reviews
This doesn’t just mean that you should be nice to people when reviewing their code. It also means that you need to be capable of understanding what your company does and why you are there.
Many misguided developers adopt the posture of the high-priest, whose sacred duty is to protect the sanctity of a beautiful temple of code perfection. In reality, you’re a craftsperon, and your theatre is a sometimes ugly workshop where you make a beautiful product. Code is not your product; your products are your products.
Avoid Hasty Selection of Technology
A chronic problem I’ve observed in code standards and review is that teams rush headlong into the selection of tools and technologies, without any consideration of the actual motivation for standards and review. It’s an understandable mistake, since that’s one of the few fun parts of the work, but it means that you’re missing the most important part of the work, which is figuring out what you’re doing and why.
The seven motivations that I mentioned earlier are a good place to start when having these discussions, but I’m sure that list is not exhaustive, and the Compliance section warrants far more than a single paragraph.
The tools and style books you use will of course depend entirely on the languages that you’re using, but there are some strategies for employing those tools that transcend language.
Ensure that the tools (editors, linters, compilers, interpreters) can implement the style rules. If not, reconsider the rule(s).
For example, can your indentation rules be formally expressed in such a way that everybody’s editor can perform it consistently?
Ensure that the tools run as part of the canonical build procedure, with explicitly declared tool versions.
Code reviews are the wrong time to do nit-picky proofreading of column width, indentation width, or trailing whitespace problems. If your tools allow this to happen, you are wasting crucial developer time by making them play proofreader.
Whatever build command developers run locally should be the same as what the production build system uses, and that build command should execute the tools in such a way that the build fails if there are code standards issues.
Further, the versions of these tools must be precisely the same on both the developer’s workstation and the production build environment. Otherwise, local successes or failures are meaningless.
Code evaluation tool versions should be pinned to the project being built, not set team- or company-wide. Otherwise, careful orchestration is required in order to perform updates to tools. Don’t expect that developers are going to have to install a bunch of stuff on their machines by hand and then keep the versions in sync with some upstream source.
A sign of the immaturity of our field is that virtually every software company has invented their own code review and style system, which means that nobody has really figured it out yet. As mentioned, the research is extensive but the conclusions are somewhat thin.
- Modern Code Reviews in Open-Source Projects: Which Problems Do They Fix?
- A Motivation Model of Peer Assessment in Programming Language Learning
- Design and code reviews in the age of the Internet
- Expectations, Outcomes, and Challenges Of Modern Code Review
- Open Source Peer Review – Lessons and Recommendations for Closed Source
The willingness of companies to share their policies and tools, and to study and document their experiences with those policies and tools, appears to be driving improvement in our industry. I believe that the psychological aspects of code review are almost completely ignored in the computer science literature however. I suspect and hope that industrial psychology also has a great deal to teach us about how to improve peer review in software development. Recommendations on research to study in that area would be much appreciated!