Don't tell me about pull requests. They don't work either
Introduction
A code review takes the form of an interruption, the type of interruption called a “meeting.” This is the first point I want to raise in my continual push to restore the recognition of Flow as the most important condition of software development. Interruptions disrupt concentration and lost concentration is lost productivity and diminished quality. And this is just the start.
Code reviews are rarely comprehensive; they are another example of checking a box and rarely catch bad practices or subtle errors. They are wastes of time and ineffectual, but they don’t have to be. We have better tools now but we don’t yet use them for reviewing code. And, no, I am not talking about pull requests.
They are however too important to be left as just another procedure. All developers have blind spots and when we develop new code we tend to focus on the expected path of execution and do error handling as an anxiety-driven compulsion to be gotten out of the way (cleanup, return) instead of considering all that might credibly go wrong.
Test-Driven Development has led the idea that developers write their own tests, a profoundly unwise assumption, making code reviews the only opportunity for other eyes to review the logic and potentially reveal its holes.
Traditional Code Review
You get called to a code review. Five minutes before the meeting you send a job to the printer queue, the changed files. Waiting at the printer are the other meeting attendees, doing the same thing, wasting paper for a single-use printing just in time for the meeting.
As the meeting attendees arrive they flip through the pages, maybe not even sure which parts are changed. They are seeing the code for the first time. They certainly don’t have time to analyze the code or the context of the changes so once the review begins the comments are mostly about superficialities like formatting or style.
Why are you checking for NULL there? If that call fails it’s going to throw and you’ll never reach the NULL check
Flip, flip, flip.
Why are you putting the && and || operators at the beginning of the line? The rest of the team puts them at the end of the line. I think we should be consistent
Flip, flip, flip.
This is excessive indentation. Why don’t you just stick in a return and Just Get Out?
Uh, excuse me, are we ever going to talk about my changes or just about minutiae?
Where are you going?
Back to my desk. This is a waste of time. (slam)
He’s not a team player. I’m scheduling a meeting with Microsoft Outlook.
This kind of review doesn’t really investigate the changes, just looks at them. The paper ends up in the trash.
If this kind of review is of a substantial amount of new code as opposed to incremental changes to existing code then a few actual useful remarks may come out of it. It’s all in front of the readers, but even then the analysis is going to be mostly superficial. Because even a completely standalone module is called from somewhere and it needs to be analyzed in context, which is not going to happen in an interruption.
Er, meeting.
Pull Request Review
A pull request (terrible nomenclature; is the request ever denied?) shows a code diff and is usually done after a review. I don’t have a lot of experience with these but code changes without the surrounding code provide no context, without which the review is even more of a box-checking exercise.
I don’t like being sent a PR link and asked for a review. It’s simply inadequate but has become, along with so many other shoddy and ill-considered recent practices, the “industry standard.”
Looks fine to me
A Better Way
Forget the interruption, er, meeting. Don’t hold a meeting. Let me tell you how I do code reviews.
I’m assuming all readers use Git and if not the same ideas area applicable to Subversion or whatever revision control you use.
There is a tool called GitKraken that integrates with your repositories and shows the branches in current and common use, and allows display of the contents of any commit in context.
screenshot from GitKraken
GitKraken is free for noncommercial use and it is a truly excellent tool that provides a lot of what is missing in Git. It can be used to operate your local respository but that is not within the scope of this document. In the screenshot above
- the left pane is a list of branches
- the central pane is a list of commits, each line representing time on the vertical axis, top being most recent, with each commit an image of the developer avatar
- the right pane shows, when a commit is selected, the contents of the change with the changes color-highlighted and some of the other file contents
The Process
I recommend reviewing every single commit. Within reason of course; you don’t need to review projects you never work with, use your judgment (if agile has left you any).
The advantages are
- You do the reviews at a time convenient to you, instead of attending an interruption and wasting a pile of paper
- Changes are color coded (insertion, change, deletion) clearly
- You can load the branch and examine the code in situ
- You can check all references to the component to see how the change will affect the call points
- You can keep up with all changes to the codebase, even those outside your work area, e.g. changes to browser JavaScript when your own work is in the server
Conclusion
This is a responsible, thorough, and contextual way to accomplish a code review without the wanton wastefulness of the traditional paper-flip interruption or the near-useless pull request review. It makes reviewing code as much a part of your workflow as writing it.