This year I’ve moved from a systems/security engineering position, sometimes dabbling in development and hacking away at some small projects, to a full-time software engineering role. It’s a welcome change, broadening my scope significantly, and I’m looking forward to continue engaging in multiple open source projects with a fresh mindset. Doing so has been a good opportunity to examine not only new processes and paradigms, but how various styles and limitations in associated communications can impact both a project, and its members. In particular, I’ve found that code reviews, both in open and closed source environments, can bring significant challenges to interpersonal and inter-team communication; these challenges are exacerbated when communication occurs between new and experienced members on a team. Following is a smattering of thoughts on maintaining helpful, productive communication as part of the code review process:
- Review comments should be imperative or interrogative. Declarative statements are often unhelpful in the context of review where verbal communication (and in-person nonverbal cues) are missing. The capacity for dialogue is limited in the context of asynchronous communication in code review. Review comments should be actionable; the code author needs to have something to go on. A comment such as “an alternative approach could be x…” is ambiguous, and doesn’t provide a clear path forward for the author, even though the comment is based in fact- yes, the statement is true, but how should the author act on this? Change the code to meet the comment’s ideas? Agree and do nothing? Leave no response at all? There is no clear way to move forward. In cases when the reviewer is leaving a declarative statement merely for edification or conversational purposes, that intent should be made explicitly clear (e.g., sugaring the comment such as “just FYI”, or “no action is needed here, but…”, or “historically, we have done… but this is not a blocker”).
- Interrogative questions should be neutrally phrased. Again, the lack of communication cues (tone of voice, facial expression, gesticulation, etc) associated with code review force the review recipient to infer tone and meaning in a comment. As with all asynchronous communication, the onus of responsibility falls on the speaker to frame their communication with the appropriate context and tone (thanks, emojis). In the context of code review, review comments phrased as a question can be particularly tricky to navigate, given the average software engineer’s penchant for never ending quips and sarcasm. It can be easy to misinterpret interrogative comments as snide/rude/hostile sarcasm, so take care to phrase review questions neutrally and based strictly on the context at hand. Interrogative comments can be useful in encouraging the code author to further explore the surrounding codebase (e.g., “what happens when X occurs instead of Y”), or engage is a useful, edifying dialogue, but they can also easily be an avenue to convey a belittling attitude. Tread cautiously.
- Reference existing documentation. Got a style guide? Great. Use it, particularly for new contributors. Does the language/environment in question have an idiomatic set of best practices? Wonderful. Point it out. Noting issues that need fixing is great, but it devalues the exercise of code review when employed on its own; review comments pointing out small/nitpick items need to equip the author with tools to prevent the same mistake. Nitpick requests (particularly aesthetic/style changes) can be exhausting, especially for new contributors; having a source of truth for such issues can eliminate the fatigue associated with multiple rounds of meticulous back-and-forth.
- Eliminate conflict over personal preference. This is another place where having a style guide is incredibly useful. I’ve seen developers get hung up on minor style preference issues (aesthetic style, mind you, not even functional/design style that would have an impact on the end product) to the point that major interpersonal conflicts developed. Obviously this is a pathological case, but the underlying cause of a difference in personal preferences should never inhibit development efforts. Developing a comprehensive style guide eliminates the possibility for such conflicts; if such a guide doesn’t exist, requests changes in style should be kept to an absolute minimum (or avoided entirely). In cases where the style guide doesn’t (or shouldn’t) cover the conflict in question, favor the baseball idiom: tie goes to the runner.
- Remember the purpose of code review. Code review is an opportunity to find bugs, provide some fresh eyes, and improve the overall quality of deliverables, in the context of the change at hand. Reviewers need to remember to keep this purpose the focus of their mindset as the review is executed. If no bugs are present, the change is relatively simply, and the quality is up to snuff, then maybe it doesn’t need to be sent back for further work. There’s a pervasive mindset among developers that, unless something in the patch set needs fixing, the review wasn’t properly/thoroughly done. To be clear, I’m not calling for reviewers to be less pedantic in their work, particularly when requested changes can be backed by documentation or historic precedent, nor am I advocating that code review be done any less thoroughly. Reviewers do need to remember, though, that end goal is to make an improvement upon the existing project, not to prove one’s worth as an engineer by blatantly refusing to accept any code changes before getting their two cents in. As Gareth Wilson put it, “sometimes it’s okay to say ‘it’s all good'”.
- Keep comments in scope. Just as changes should be atomic, so should review comments. Asking for changes in untouched or unrelated pieces of the codebase is unhelpful, and breeds an attitude of objectionism (see above). In the same vein, standalone backhanded/sarcastic comments about technical debt (“*sigh*”) do nothing more than paint the reviewer as a griper (not that sarcasm doesn’t have its place when it’s appropriate and properly construed).
- Keep it calm. Code review should never be treated as a competitive exercise; too often reviewers and authors forget that they’re playing for the same team. Attitude polarization can quickly poison a constructive dialogue associated with a review. Heated exchanges are rarely productive; it’s far often better to table a discussion rather than pursuing the opportunity to prove oneself right (or alternatively, to simply prove one’s “opponent” wrong).
- Remember the context of the review. The attitudes, experiences, and histories of the code author and the code reviewer are likely to be different, to the point that conflict is eventually unavoidable. This contrast is more starkly highlighted when reviews involve a new/junior contributor, and a senior reviewer. I’ve observed on multiple occasions, in disparate development environments, that senior members of a team are more unwilling to accept change requests from new contributors than other experienced team members, merely on the basis of the authors lack of experience. Even held subconsciously, this attitude stifles development efforts on both sides, as the catch-22 associated with new contributors breaking the preconceived expectations of senior team members results in a zero sum effort. Likewise, it’s imperative that new contributors remember that experienced project members have understand and experience of the project that far surpasses the author’s. Dismissive cynicism from either party is a major blocker to a productive engagement; new authors and experienced reviewers must be mindful of the others stance, and work cooperatively to meet their end.
Software is hard. People are hard. We’re never going to get either of them right, but we can all work to understand them a bit better, and be mindful of how our approaches and communication impact both.
