How to get developers to do code reviews in a timely manner

The company I work for requires all code to be reviewed by other developers before it is committed. The members of my team are often frustrated because the other developers are too busy coding to do a review, especially if it is very long. How do you incentivize other developers to do timely code reviews?

(We use git-svn so we are able to continue coding while waiting for a review. However, I still find it frustrating when I have to wait a long time before I can commit my code.)

Look at how facebook does it with their own app, called phabricator: http://phabricator.org/

They basically commit on a per issue basis, and for each issue, the code is shown, which is to be reviewed by someone. The code doesn’t go into their main repository until the reviewer said it’s ok to do so.

I guess it makes it more fun.

Also, perhaps a code should be assigned to two people: one who does it and one who reviews it.

Albeit perhaps your teammates don’t believe in this review thingy.

Personally, in lack of reviewers, I used unit tests for lower-level functions and “the janitor test” for all the rest: the janitor test is called that way, because even the janitor should be able to understand your code.

I usually removed some minor parts, like block / function scope brackets, visibility notations, sometimes even types, and showed it to managers, domain experts, mates, whoever requested the code: “is this what you want?”

Also, going there personally and not leaving until reviewing is done helps.

Or, in case you’re not fine with the team, or they’re not fine with you, you know, “if you can’ change the company, change company”…

I’m going to base this on a couple of assumptions:

Everyone seems to want to write code (If not, you have people that need to go.).
Everyone wants their own code to be checked in.

Only allow those who complete their reviews to have their code checked in.

Maybe a certain block of time can be dedicated to code reviews in hopes of avoiding the problem of being interupted.

The goal should be to check in quality code. You don’t want to punish/force the reviews to the point where everyone just gives it a “rubber stamp” approval.

If you have different levels (jr., sr. etc), promotion and maintaining a title should be contingent on doing your job.

At a couple of previous employers, we rotated who was on Code Review on a daily basis. Usually 3 people shared a CR day and each CR had to be signed off by two of the reviewers. This made sure that when it was your day, you knew that CR was expected of you, regardless of your other projects. So every five days or so, it was your turn and you could adjust your development tasks accordingly.

Currently, we have a Team Lead do a cursory CR on their team’s code. Depending on which area of the application has been updated, after the CR is completed, it can be bumped up to the Global Review Team, where we check for things that have a global impact on the application(s), instead of things that are related to a single feature. When there is a large Review to be done, we usually break it up between a few people so no one person has to deal with changes across a ridiculous number of files.

That said, we are only ever reviewing code that’s been committed to the current Dev branch / variant. The code has to pass Code Review, Global Review, DB Review and UI Review (each as needed) before it can be promoted to the next (e.g. Alpha) environment.

Finally, we agree to an SLA on how quickly Reviews are turned around. Hardly anything’s in queue for more than 48 hours and most reviews are done in less than 24 hours.

The company I work for requires all code to be reviewed by other developers before it is committed. The members of my team are often frustrated…

Unless you’re doing mission critical software or critical patches to production release candidate code, there are no compelling reasons to rigidly stick with particular type of code reviews.

^{The idea behind your company requirement sounds reasonable on a surface (100% reviewed code) but the means they decided to use are counterproductive – because as you point out, these lead to developers being frustrated.}

Walking in your shoes, I would first point the management that post-commit code reviews are considered equally as respectable as pre-commit ones. These terms are searchable on the web – if needed, find there references to backup this. One does not necessarily need pre-commit reviews to get 100% reviewed code.

Based on above, I would next suggest them to drop micromanagement attitude and let developers try the way that feels more convenient to them. Pre- or post-commit reviews are best left for programmers to choose. If company knows better than programmers, why don’t they write the code themselves?

You have a number of issues to address – you have to win hearts and minds and you have to ensure that time is available for code reviews.

The second part is probably easiest – you agree (collectively and that has to include management) that the first thing a dev does each morning is their code reviews – this is simple, understandable, effective and gives you a nice clear stick to beat people with if they don’t comply. Better, you’re not interrupting anything, you’re not asking them to stop working on their code you’re not asking people to squeeze something into their to-do list…

The first part is the real problem – the participants in the review process have to see it as having value otherwise they’ll never do a code review (which is percieved as not having value) when they could be writing code or fixing bugs (which is surely more important…?).

If you can put the two together – firstly ensuring that everyone believes (or understands) that there is value in the code reviews – at the most basic it should mean fewer bugs which means more new code which is usually more fun – and then secondly arranging things so that there is a clear space in the schedule for the code reviews to be done then hopefully good things will happen… it will become part of the culture.

Once its part of the culture it may no longer be necessary to say “first thing every day” – but having said that I think that it fits well into the pattern one probably wants a dev to work in.

At most companies I have worked for, you have 3 days to complete a review. It isn’t acceptable to not do the reviews. It’s part of your job. If you don’t do decent reviews on time it affects your performance appraisal. And yes, reviews always seem to happen at the most inopportune times. Too bad, learn to include review time in your estimates. Anyways, if management truly believes that reviews are important (ie. they mandate that all code is reviewed) then they would push a similar policy. Additionally, if people don’t complete the review in the allotted time, that goes as their acceptance of the material.

Consider using a tool like Review Board. It is very helpful, especially for long reviews.

You can upload your diffs and wait until a reviewer has finished their review.
If you have open reviews that prevent you from continuing your work you can report this during your daily meetings (your team want new features to be checked in so that they can be tested as soon as possible, don’t they?).

A few points to add that aren’t in the other answers.

The code to be reviewed must be checked in

so that you are reviewing a stable version.
It can be on the main development branch if a release is far enough away
It can be on branch if there is a good reason not to pollute main

Blocking tasks take priority, therefore code reviews should take priority over other work (but trying not to break your flow).
As a developer you should want others to review your code (as you are aiming to make it better). In that knowledge you should perform reviews for others promptly.

The harder questions are when and how to do code reviews well.

A rule that has worked for us on the when is that shared code must be reviewed as it has wider impact whereas in code for a single application (especially given we are using test driven development) its less important.

Filed under: softwareengineering - @ 13:56

Thẻ: code-reviews

Thiết kế website giá rẻ

Danh mục

How to get developers to do code reviews in a timely manner