Lessons From the Programming School of Hard Knocks

If you’ve been writing code professionally for any length of time then in all likelihood you’ve been required to submit your code for review to another developer or set of developers. If you haven’t had to then congratulations!, you’re that lone developer writing code in a vacuum, and this article is not for you. If you’re not that lone developer then you know the pain and angst that can come from the review process and you’ve come to dread it as many of your peers have. Read on for tips on how to make the code review suck less.

Lets get the easy piece of out of way and talk about using automation to streamline the review process.

Maintain and Run a Beautification Script

Your shop probably has a coding standard of some sort already in place, and scripting a beautification step according to an existing standard is an easy first step to simplifying reviews. This eliminates petty review comments which focus on stylistic deviations, such as the age-old “curly braces go on the same line or the next line” argument, allowing reviewers to focus on more important matters. It also serves to educate new developers on what the coding standard is without rubbing their nose in a bad review like an non-house trained dog.

Ideally the script should be run automatically as part of the review submission. This lets the submitter or submitters (if your shop practices paired-programming) focus on solving the problem at hand, rather than constantly thinking about how they’re going to get nitpicked in the review for minor stuff like style and formatting.

Use a Code Review Tool

Another way to automate the review process is to use some sort of code review tool. The point of the review tool is to facilitate discussion and keep a record of the review comments and changes that the developers make in response to those comments. The days of passing around a patch file via email have long since passed, and there are any number of excellent code review tools out there. Ideally whatever tool you select should:

  • integrate with your VCS of choice (You are using version control aren’t you?)
  • view diffs of the old and new code
  • allow users to make comments
  • be notified of new changes
  • be notified of new comments
  • link to a specific line of code in a given file

Dot Your “I’s” and Cross Your “T’s Before the Code Review

As a submitter you should do your due diligence and make sure you have covered all areas of concerns. Do your tests drive out all of the functionality (You are practicing TDD aren’t you?)? Did you implement the required functionality? Is your code efficient? Does it fit into the existing architecture?

Basically you should do everything in your power to insure that you aren’t going to fuel the fire and get burned in the review. I know this sounds like a “eat your vegetables and brush your teeth” kind of point, but doing these things preemptively can take the fight out of a review before it even starts. As most developers are well aware there is no shortage of reviewers who take an unholy delight in pointing out how terrible someone else’s code is, and it pays to make them work for it.

Keep Submissions Short

Code reviews go a lot more quickly when they are small. The fewer files and diffs a reviewer has to look at the quicker they can perform the review.

If you’re working on a particular feature and feel like the changeset will be huge your codebase is telling you that either a.) you have a design problem (Small changes should be just that; small. If you have to touch a large number of files to do something simple you’re probably doing it wrong.) or b.) you haven’t broken your work down to the right incremental unit. The name of the game is to integrate early and often and the size of your changeset and code review should reflect that.

Complete Reviews Quickly

When it comes to reviews it’s as Egon Spengler would say: “The door swings both ways.” Reviewers have their own set of obligations to meet. Chief among these obligations is to complete reviews quickly.

If you’re a reviewer and you’re letting the changeset languish in code review hell for hours or days a time then guess what; you’re part of the problem. If you insist on being on every review and don’t make time to complete the reviews you’re part of the problem. If you refuse to approve a review over a matter of opinion you’re part of the problem.

Bottom line: As a reviewer you have an obligation to get the review over and done with quickly, and to not hold up development any longer than is absolutely necessary.

Check Your Ego at the Commit

This one is for submitter and reviewer alike. As a rule there is a fair amount of ego involved in writing code. In a sense, as developers we’re playing god in our little piece of software; creating the laws of our tiny universe, defining what the universe looks like, setting things in motion and how dare another dev tell us our code is wrong?! Its obviously a flawless creation for which there can be no other implementation.

On the other hand when we review another developer’s code it’s easy to see how wrong someone else might be and how dare they not acknowledge our expert opinion on this matter?

Both stances are of course wrong. As developers we need to leave ego out of the code entirely. We need to be able to acknowledge that we could be wrong, or that there might be a better way to approach the problem. We also need to remember that the submitters are our fellow co-workers and that the next review could turn the tables, as again “The door swings both ways.” Speaking of which…

Rotate Reviewers

Code reviews should not be a one-way street. If only the junior developers are submitting and only the senior developers are reviewing then you’re doing it wrong. All members of the team should have the opportunity to be in both roles, and different submitters should be sending their reviews to different reviewers on a regular basis.

I can imagine some of you shaking your heads and wondering how junior developers should be expected to review a senior developer’s code; if so please scroll up and read my previous point. If you’re a senior developer on your team then your job includes coaching and helping the junior developers to learn to be better developers. One of the ways to learn this is to read lots and lots code, and code reviews are a perfect opportunity to do this.

Come Down Like the Hammer of God on Unprofessional Comments

This is a pet peeve of mine, and I bring it up because this is one aspect of the code review that needs to put down hard and fast. Reviewers who feel the need to fill their comments with insults and obscenities (even the acronymized sort) have no business being reviewers, and no developer should have to tolerate such behavior. Comments should be kept above the belt and focused on how to improve the code. Developers should put their foot down at the first hint of this kind of behavior to prevent it from occurring again in the future.

Have Face-to-Face Conversations

While this isn’t always possible, having the code review performed with the submitter and reviewer in the same room at the same time is usually preferable, as it cuts down on a lot of communication problems (see the sections on ego and unprofessional comments above).

I’ve found that that further away submitters and reviewers are located from each other the worse the communication (and therefore the reviews) can be. This is especially true if there is a remote team involved, and so it becomes imperative to communicate face-to-face as much as possible. This has the effect of reminding all parties that there are actual people attached to the code, and the often demeaning comments we see in reviews simply aren’t made to someone when you’re face to face with them.

Conclusion

Hopefully you’ve read this article and come away with some ideas on how to make code reviews less painful. A lot of what I’ve talked about here requires cultural or organizational change at one level or another, which means it won’t happen overnight, but some of these points can be implemented at the team level.

If you find yourself loathing the review process I encourage you to be the change you want to see, and start doing those things which are in your power to improve the process. Good luck, and thanks for reading!

Did you enjoy this article? If so please recommend it, and check out my other articles . If you have any thoughts about this article I’d love to hear from you. You can find me on Twitter as @jameskbride. Thanks for reading!

Exported from Medium on December 25, 2015.

View the original