A code review checklist prevents stupid mistakes

Blaine Osepchuk on October 23, 2017

My team uses a code review checklist to prevent stupid mistakes from causing us problems and wasting time. In this post, I want to share the reas... [Read Full]
markdown guide
 

Good article, but can you please not use "he" as a default pronoun for programmers (or anyone, really)? "he or she" or "they" would be much more inclusive.

The author of a pull request does a self-review on his code using the code review checklist. He corrects any issues he catches and then releases his pull request for review.

 

Thank you for bring this up. I've seen this handled lots of different ways in books and on the web and the system that I think makes most sense for me is to alternate between male and female pronouns--one post with male pronouns, the next with female pronouns, repeat.

 
 

We do something similar to this, but we have a probot (probot.github.io/) that inspects the PR for file patterns and includes different checklists where relevant.

For instance, if there is a database migration it will include:

  • [ ] This PR appears to migrate the database schema, have you considered what indexes will be needed?

Or if a controller or model is changed:

  • [ ] This commit modifies a controller, are you sure that this commit does not break external API clients

I think this really helps with the utility of the checklist, otherwise it quickly turns into noise.

 

Interesting. I had never heard of probot. I'll have to check it out.

 

this specific checklist looks like enterprise bull$hit to me. it sounds more of a way to intimidate/coerce the devs than to actually help them out.

 

Okay. That's not our intention.

I work in a small company as one of two programmers and we made the checklist ourselves so that's about as far from "enterprise" as you can get.

 

We've done something similar at a start-up I was at. Well, it was between start-up and whatever is next with around 30 devs by this time. The idea was to encourage people to not just rubber stamp reviews.

One missing thing is to check out the branch when you are reviewing (if the code bases isn't tattooed in your brain).

Yes. You must checkout the branch and examine it in your IDE. Otherwise you can't do several of the checklist items like check for inspection errors, run the tests, etc..

 

I enjoy a good checklist but I understand this guys reaction! You gotta have some pain before you want to take on an extra step like this.

 

Hi Blaine, great article. I think your proposal works but missed another use of PRs, instead of be focused on "approve this piece of code" you could use it as a tool for design/discover/discuss about the solution. I think CR is a way to communicate with my team,anyway love the idea for a "merge to prod" proposal.

 

Of course you can. We do that too but it wasn't the topic of my post. Thanks for commenting.

 

Great article, thanks for sharing!

When you described how much time was wasted on failed code reviews, I was surprised. Why does a failed code review cause so much time to be lost compared to if they submitted the proper code the first time? Can you elaborate on that?

 
 

This was inspiring for me, especially the detailed process described in the comments.

We are using github and are having two checklists in our pull request template. But they are much more technical.
Depending on what kind of (trac) ticket you worked on one of them might apply.

We already realized multiple times ;) how important self review is.

We consider a PR ready for review as soon as somebody is invited for review in github.

 

Thank you.

There are many ways to handle these flows and I'm not asserting that ours is correct or someone else's is wrong.

We rely on the Jira sprint board as our official way to telling the status of a story for a couple of reasons:

  • we get tons of bitbucket emails and we find it hard to see the status of a story or pull request among all the noise.
  • review isn't the only reason we create pull requests. We have a 'feedback' status where the author makes a PR, adds reviewers, writes questions or concerns on the PR and then moves the story into 'feedback'. When the reviewer sees a story in feedback he/she looks at the PR and tries to help the author with the story
 

Thaks for the article! My team finds value in all kinds of checklists, too. The team at my last place found it hard to handle those, especially the over that should be filled out for every commit. So I went and made an online checklist that could easily integrate in source control and task trackers.
Feel free to use it, too - everyone can create public checklist templates. The results are not public, though:
guess-what.io/c

 

Great Article !

We are doing code review on the basis of coding convention. And some of the issues can be solved by using the tools like codeclimate.

I am not even thinking about code review checklist, we will make those checklist and try it out ! Thanks :)

 
 

My team of 3 used a checklist very similar to this for over a year. We ended up abandoning it because most of the time the team was just zombie checking off the list. How do you avoid checklist fatigue?

 

We had the same problem (zombie checking things as done). We use two google forms and our "quality discussions" to solve the problem.

The first is mentioned in the post. The reviewer records the results of the code review checklist in the google form. During the sprint retrospective you basically get teased (gently) if you miss a checklist item in your self review. Stuff like, "Hey Bob, do you need a new checklist to pay attention when you do your self reviews?" That has been surprisingly effective.

Second, we track defects that we catch in production (and that are reported to us in production) in another google form. We look at each of these defects during our retrospective and do a root cause analysis. We want to know how that defect got by our process. Is it a problem with the process or did someone fail to follow the process? Tracking defects in production helps us detect if both the author and the code reviewer missed the same checklist item (it hasn't happened yet).

Third, we frequently talk about quality and how the cost of fixing a defect sky-rockets the more development stages it gets through before it's detected. We can afford to spend an extra few minutes or hours to make sure the code is good (or at least not obviously bad) because we aren't spending 50% of our time chasing defects in production. In other words, we are happy to invest an hour up front for the chance to save 10-20 hours downstream.

 

This is weirdly relevant, I just started working on a Node CLI tool for running thru checklists because I also found myself making less dumb mistakes when I have some kind of checklist before commits or releases.
Here's the repo github.com/SammyIsra/ChecklistChecker
Its still not finished, but it may be worth to check out 😉 I intend to work on it thus weekend, so I should have it done sometime then!

 
 

We've been looking for a great way to integrate the theory of constraints into our programming practices. Love the way you take the books that you read and create actionable steps

 

Thanks.

If you like Theory of Constraints, you are going to love "The Principles of Product Development"(Donald Reinertsen)). It's an amazing book for any software development team.

The basic premise is that agile/scrum/whatever methodologies borrowed concepts from lean and six sigma without realizing that the fundamental assumptions behind these manufacturing methodologies don't hold in product development. This book points out the bad assumptions and gives you remedies that will work for product development. Once you read it, you'll be shocked at how silly we've been for blindly borrowing the ideas from manufacturing.

I've got a whole series of posts planned around this book but they won't be ready for a while.

 

Thank you kindly for recommending a book that I suspect may blow my mind. I appreciate it!

You're welcome.

He's also got some great overview videos on youtube:

 

Hi Blaine, great stuff - thanks for sharing! I've got a question about "releasing" a pull request for review. How do you handle this, practically speaking, in Bitbucket? Does your team create pull requests, then conduct self review, then add reviewers? Or does self review happen before you create the PR in Bitbucket?

Does a successful code review result in a PR "approval", or your processes for review and approval largely separate?

 

Hi Christian. Thanks for reading.

We usually do it like this:

  • create a story in Jira (add an estimate, description, assign to a programmer, etc.)
  • programmer moves the story from 'to do' to 'in progress' in Jira and then creates a feature branch for the story
  • programmer makes code changes to satisfy the story (along with a testing plan document, unit tests, etc.)
  • programmer does a self-review, runs the unit tests, and executes the manual testing plan. Programmer works on the branch until he/she is satisfied that it meets all requirements (or moves it to 'feedback' in Jira to ask for help)
  • once the programmer feels the branch is ready for review he/she creates a pull request in bitbucket and adds a reviewer. The programmer reviews the diff in bitbucket. I don't know why it's different to look at the diff in bitbucket vs our IDE but we usefully find something to fix while looking at the bitbucket diff (often documentation but it's still important to us).
  • Now that the branch has been self-reviewed, and reviewed by the programmer in bitbucket, they move the story to 'review' in Jira (this is the "release for review" step you were asking about)
  • The reviewer knows to review the pull request because they are the assigned reviewer on a pull request for a story in the 'review' column of the active sprint board.

A successful review does result in a PR "approval". But we take it a step further. A reviewer who approves a pull request does the following:

  • approve the pull request in bitbucket
  • fill out our code review Google form
  • merge the branch into master
  • deploy the branch into production
  • move the story to the 'done' column in Jira

We think the immediate merge and deploy steps are important for two reasons. First, it signals to the programmer his/her code better be "done-done" when they put it into review because that is their last chance to change it before it goes into production. And secondly, it gets the code into production as soon as possible so it can start delivering value to our stakeholders.

Does that answer your questions? How does your team do it?

That's great. I hope you find it as useful as I have.

 

Thanks for a great article. I'm curious, how long on average does a code review take for your team? How do you handle situations of reviewing "large" PRs?

 

You're welcome.

The research says that after an hour the effectiveness of code reviews falls off a cliff. But we are running an experiment right now where we are shooting for a max code review time of 40 minutes and that's looking even better.

I wrote about it here.

code of conduct - report abuse