14

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.)

8 Answers 8

12

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"...

9

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

  1. Everyone seems to want to write code (If not, you have people that need to go.).
  2. 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.

1

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.

1

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?

4
  • 1
    "If company knows better than programmers, why don't they write the code themselves?": very good comment! But I would hope that development managers are former developers themselves.
    – Giorgio
    Commented Jul 26, 2012 at 9:57
  • 3
    Post-commit hurts your code quality terribly in my experience. Programmers are lazy, and they will feel they're done if it can be committed: "yeah, well, it's not perfect, but who the f.ck cares, what's perfect in life? it does the job, doesn't it?" the only good answer is NO, and perhaps the managers have a more realistic image of programmers than they have about themselves, that's why they require pre-commit (or at least, pre-merge) code reviews.
    – Aadaam
    Commented Jul 26, 2012 at 11:58
  • @Aadaam your experience definitely differs from mine - not only with regards to post-commits, but also (and especially) wrt part of "Programmers are lazy..." As for managers having more realistic image I agree that was typically the case in my teams; it only did not led managers I used to work with to senseless ideas about what kind review to force
    – gnat
    Commented Jul 26, 2012 at 12:36
  • Well, I worked in outsourcing. In outsourcing, most of the programmers aren't in because programming is fun, they are in because programming has the best work/pay ratio, with rates much higher than anything else... they hate it like any other job.. and they try to do everything to further "optimize" this ratio, if you know what I mean...
    – Aadaam
    Commented Jul 26, 2012 at 13:09
1

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.

4
  • You can't truly agree that "first thing every day" rule in the first place. If someone finds a bug that costs the company X dollars per hour (or increases the risk of missing an important deadline by X percentage points per hour), and they happen to do so five minutes before you get in, then code review is not your top priority. Basically the problem is the clash between the desire to set simple rules, vs the fact that priorities are not always simple. The risk is that everyone will whole-heartedly agree the rule, and within 24 hours find some reason why today is an exception to the rule. Commented Dec 18, 2012 at 23:58
  • And the solution is tricky, but IME you have to find enough "space" to introduce a new working practice that is time-consuming but worthwhile. This requires foresight to identify slack time, a willingness to let deadlines slip as the price of introducing a worthwhile change, or both. TANSTAAFL. Like you say, once everyone is settled into the pattern they can make exceptions. Hopefully they do so based on a proper appreciation of the value of the general pattern. Commented Dec 19, 2012 at 0:02
  • I say "let deadlines slip", I should have said "move deadlines". "slip" implies moving them after they're committed, i.e. failing, but it needn't be that way. You could instead plan for a period of slightly reduced productivity due to the inflexible new rule (and the inevitable inefficiencies caused by people trying to follow any new process -- if you're doing code review first thing then you'll miss the morning scrum meeting on days the code review takes too long, or whatever unique niggles your organization can throw into the mix). If it's a good rule you'll soon get above where you started. Commented Dec 19, 2012 at 0:11
  • @SteveJessop of course you can truly agree that. And of course there are going to be exceptions (I happen to think the scrum observation is silly though - especially as the answer is obvious (-: ). I think the key is that there is no "one size fits all solution" I just proposed something that's simple and easy to understand and is relatively hard to bump of one's schedule (again depending on the environment)
    – Murph
    Commented Dec 19, 2012 at 10:09
1

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.

0

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?).

0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.