Confessions of a programmer: I hate code review
Most of the projects I’ve been working on today have fairly strict code review policies. My work requires code review on most of our code, and as we bring on an army of interns for the summer, I’ve been responsible for reviewing lots of code. Additionally, about five months ago BarnOwl, the console-based IM client I develop, adopted an official pre-commit review policy. And I have a confession to make: I hate mandatory code review.
I should be clear that I think I still believe in code review as a useful tool for developers working together on a project. A reviewer will flag as bad style or inefficient or ugly things that you let slide working for yourself. A reviewer comes at code without the assumptions of how it’s supposed to work that you made, and can often spot errors you made because mixed up a mental model of your intent with the code you actually wrote.
In addition, code review is a great way to ensure that at least two people are familiar with each piece of your infrastructure. There is often a natural tendency for different developers to take ownership of specific pieces of code or infrastructure, and be the only ones who touch or maintain it. But then it breaks while they’re on vacation, and everyone else is left trying to figure out how this code was ever supposed to work. A mandatory code review policy is often a great way to mitigate that class of problem.
But, theoretical and observed benefits of code review aside, speaking personally, as both a developer and as a reviewer, I’ve been finding it a really frustrating process.
As an author
As a developer, I hate that code review adds unpredictably long latencies into my development workflow. Once I’ve finished and tested a feature to my satisfaction, and sent it off for code review, I have to wait for a potentially long time before I can actually mark it as done. This means that, when deciding what to do next, I have essentially three choices:
- Busy-wait. Get coffee, read reddit, and check my mail until the review request comes back.
- Continue development on top of the code I just sent for code review.
- Work on something completely different.
All three of these options suck. (1) is convenient if I can expect the review to come back shortly, since doing something idle like reading reddit or checking mail lets me keep the code in the back of my mind, so I don’t have to page it back in when the review response comes back. But obviously it’s inefficient, wasted time, and unacceptable if I don’t expect a reply within an hour at most.
(2) is often what I want to be doing. I’ll often be working on a project with several logically distinct but related stages. It often makes sense to send out a review request for each, since they can be deployed and reviewed separately.
However, if a reviewer comes back with significant comments on the code I sent out, I now not only need to update that code, but I also need to rebase the work I’ve done since then on top of the result, which may be a real pain if I’m working on something closely related (e.g. if my work used APIs I built previously, and the reviewer asks for changes in those APIs in some way).
Option (3) avoids both problems, but means that I’m continually swapping different projects in and out of focus. This slows me down, since I have to constantly re-remember where I was in each project, which APIs I was using, and so on. Any developer can tell you that they hate switching between different projects too frequently.
Option (3) might be more tolerable with large projects, where development/review cycles are on the order of weeks. But those aren’t the projects I’m working on.
As a reviewer
Reviewing code is one of those things that I would enjoy if I had infinite time, but that I find a nuisance when I don’t. I do enjoy reading other peoples’ code and figuring out how it could be better, and giving feedback to help get it there.
But doing so well takes a lot of time, and a lot of time is something I rarely have these days. In addition, because of my above complaints about dealing with code review as a developer, I’m always acutely aware that someone is probably waiting on my reply in order to get work done. So I always feel rushed to reply to code review requests, as well.
So, even though I always feel like code review should be something I’m enjoying, I tend to find it a frustrating experience where it just feels like a task I have to do before I can get back to real work.
This is, of course, in part just a symptom of being too busy, but code review makes it a task that bugs me more than other time drains. If a customer reports a critical bug that I have to drop everything to investigate, I’ll be annoyed, but I at least feel like I’m doing something important that fixes a real problem and hopefully ends with a happy customer. If I spend a day doing nothing but reading code reviews, I’ll end up feeling unsatisfied and unproductive. Because code review feels fundamentally optional — even though I believe it’s beneficial, it’s something we’ve chosen to do, not something that we absolutely have to do in order for the project or business to keep operating — it’s more frustrating to find myself spending a large amount of time on.
In conclusion
I believe in code review as a powerful tool. But in practice, I’ve found it frustrating to work with. I’d like to hear your thoughts: Does code review work for you? Am I doing it wrong, in some ways? Is it just a question of changing my attitude in some way?
I know that a lot has been written about doing code review effectively. I’d appreciate pointers to anything you’ve found particularly compelling.
A few things come to mind for me. My company has a general requirement of pre-commit code review, which I personally find to be a net positive although sometimes frustrating.
Our system has the ability to do “to-be-reviewed” commits (assigned to a particular reviewer) on the relatively rare occasions where submitting the code is more urgent than getting it properly removed. This is “enforced” by having the “TBR” reviews continue to show up on the dashboards of both the reviewer and the code author until they are reviewed (as well as infrequent automated reminder emails). Having this “escape hatch” is helpful, though it’s not used that much.
My project specifically has a general understanding that code reviews should be high priority; a change sent out for review should really be responded to (with at least an acknowledgement) that day, and ideally faster: most people on my team treat reviews of small changes as near-interrupt priority. When everyone does that, it makes things much smoother, though if only some people prioritize code review, it can be painful.
One nice thing about our system is that it is really easy to show somebody your current work even before you “mail it out”, so getting pre-review is easy. My current project encourages pre-review for anything that requires major design / new APIs / etc. That helps for the problem you described with case 2: while I do frequently work on code which depends on under-review code, I try to avoid getting in the situation of needing to do this on top of code that will need to be significantly modified by getting a pre-review of “risky” code.
As a member of Ksplice’s army of interns, I’d like to offer my two cents.
This is the first job I’ve ever held where I had proper, formal code reviews as a matter of policy. I can say without any hesitation or question that they have made me a much better programmer and that the product is much better for them.
I have definitely experienced the frustrations you have mentioned, however, but in practice, I manage, primarily with solution #3. I find it useful to have three big “projects” I am working on, and, throughout the day, basically cycle through them round-robin style. That is, I send one off for code review, begin work on another, send it off for code review, begin work on a third, etc., and then continue in the loop as the reviews come in. Depending on the size of the project, this works well, and means that I have to do relatively few context switches.
Less Painful Code Review Practices:
Working at a large fortune 500 company that enforced these policies made code reviews manageable and painless. I enjoyed the feedback when it came verbally and was able to quickly acquaint others with my code. Additionally, because it was enforced that all code reviews were of drop-dead priority, it never once held me up.
I have a solution for you as, I, too, do not like, nor am I interested in my code being scrutinized by others.
The solution? I work on small-scale projects BY MYSELF or am the Lead Developer on a project of one – it works for me; no code review, no fuss, no mess…..I am responsible for the entire application from the beginning to the end….works for me.
I also find code reviews to be annoying. However, the amount of bad code avoided in our codebase far outweighs the annoying factor.
Also, it has proved to be a good tool in training the less experienced programmers with both general programming skills (IDE, OOP, debugging) and familiarity of our codebase’s architecture.
At my work, code reviews are mandatory, but they rarely slow me down much, because the answer to “Do you have time for a code review?” is almost always “Yes.”. I suppose it would be harder for a distributed team; here, we just walk into someone’s office and ask.
Few pointers that come to my mind:
discuss your design upfront with someone in the team. recruit them to be the reviewer. therefore you don’t have to discuss design decisions during review. if something changes, keep them posted
in extension to previous point, do pair programming. code review done on the spot.
have unified coding conventions, that way you only have the design and implementation concern (eg. was this a good solution? could it’ve been done in easier way?). Together with points above, you don’t even have to discuss design decisions, they know already everything.
I too suggest doing review on shared screen. communication is more imminent, feedback is constant
My suggestion:
Use a distributed SCM. Let code reviewers only pull the revisions they like.
Would you consider try pairing instead?
It eliminates the long feedback cycle and replaces it with an exceedingly short one. Imagine if you could get an incredibly detailed code review of your as you’re writing it. You would get all of the benefits of a code review, but without the painful waiting period. A good pair can do that for you.
That said, there are other issues with pairing (settings up workstations correctly, developers with bad breath / belligerent tendencies, etc.); but a good coach can help you through these.
I think the benefits of pairing address the problems you point out with code review here. I’ll grant that it takes some effort to get right, but I’ll assert that the effort is worth it.
4) Local branch for each feature.
@Matt I do use git for development everywhere, and use local branches heavily. That doesn’t change the fundamental problems I described with options (2) and (3) — the cost of rebasing on top of changes after feedback from a reviewer, or the cost of context-switching between unrelated projects. Local branching makes both options slightly technically easier, but doesn’t change the fundamental issues.
@Matt: I think your “4″ is the same as Nelson’s “2″. The problem is that if the next change depends on the currently-reviewed on, and the code review takes a while and suggests major changes, your new work may have been wasted, regardless of what technical means you use to implement “working on the next feature”.
Like @Tattoo my team currently does pairing with a task breakdown and design before attacking a problem. It keeps shared understanding very high.
However, when that’s not possible (distributed team, “odd man out”) code reviews are a very useful tool.
I’ll say this much: if you’re waiting more than a couple of hours, that seems to me like an early sign of a problem. Presumably someone on your team will be reviewing, and reviewing is a known part of your process. Like @Flint said, code review is “interrupt” priority.
If reviews are short (one to two hours max), busy wait is actually a very good option, assuming the “busy” part involves things of a professional nature such as keeping on top of industry trends and trying new technologies.
Which leads me to assume that you are making relatively small changes for which you would like quick feedback. But quick feedback is not always available. I’m wondering if the team dynamic is not quite right – it almost sounds like the priority of performing a review for a team mate needs to increase.
Another option would be pairing – the key benefit being the instant feedback.
But if in doing a code review for one of your team mates you could have prevented that bug from being sent to the customer in the first place, wouldn’t finding it provide you with the same level of satisfaction?
Why not pair on the code itself rather than finish it and then submit the request ?
Pair with someone on the code and then you do not need the review!
Mujtaba Hussain
P.s. Also, make sure you write tests :P
Have you tried using Git locally to manage your workflow? Send request for code review, branch, do related work, make changes from code review, merge branch.
In above comments, I can see calls for “Review at interrupt priority”.
Seriously?
You can only do that if you are not already coding something for what you indeed need to use your brain, or you will be likely to lose at very least 2 hours simply because of this interruption.
Not something I would call very effective.
At the start of a day, focus on cleaning up any left over code reviews BEFORE you get back into the mindset of something else.
This leaves the potentially longest delay as a day.