3

"Code review" (aka "peer review") seems like a really great idea, so my team started practicing it.

For a little while it worked well, but then a co-worker merged a branch in, and asked for a review of the code. When I went to review her code, the Github diff page was about 420k pixels in height. Given that my screen is about 500px, that works out to 840 screens worth of code to review.

To read the code, fully "grok" it, and write appropriate comments, I probably need an average of one minute per screen, which works out to 14 hours. Now to be fair, some libraries got checked in to this commit, so some portion of that can be skipped ... but even if libraries took up 6 hours worth, that still leaves me with an entire day spent reviewing this merge.

That can't be the most effective use of my time. And this is just one merge; we will no doubt have other large merges to review in the future as well.

So, my question is, what can I do (either in terms of procedure or in terms of utilizing review tools) to let our team have code reviews of branch merges, while at the same time not eating up entire days on reviews?

  • 5
    It's relevant because, if you're spending 14 hours to review code that took 14 hours to write, that's one thing, but if you're spending 14 hours to review code that took 1400 hours to write, that's quite another. The size of your screen is a prominent feature of your question, so it makes me wonder if you're asking about your tools or your process. – Robert Harvey May 22 '14 at 19:14
  • 9
    The obvious answer to your question seems to be to get people to merge a bit more often, or merge smaller bits of code. Aren't large merges like this already problematic, irrespective of the code review process? (conflicts, etc) – Robert Harvey May 22 '14 at 19:25
  • 4
    What benefit of code review are you trying to get, if not code quality? – Steven Burnap May 22 '14 at 21:29
  • 3
    Saying that breaking up code reviews into smaller bits which will ultimately take more time for the same amount of code is absolutely true. The issue is long term quality and responsiveness to user needs. I used to do waterfall development with a 6 month lead. Now I do Agile where we release every week. It works much better for the business – Michael Durrant May 23 '14 at 11:29
  • 3
    @machineghost Code reviews seem like a really expensive way of taking care of A, B, C, and D. The cheapest way to inform the reviewer what the reviewee has been doing is to just say it. Newer programmers will learn more from doing (e.g. pair programming with a mentor) than passively sitting at a long review of code they're not particularly invested in. Compliance to coding standards can be automated with tools. And isn't coherence of design mostly determined by the division of tasks? – Doval May 23 '14 at 13:17
4

This is actually one of the benefits of introducing code reviews in your organization: getting people to make small, incremental changes. Just wait until the guy that made that huge commit has to review a similarly sized commit of yours.

Other, more constructive ways of getting to the same result without starting a "commit war", could be:

  • setting a soft upper limit to merge sizes, as suggested by others
  • do not allow changes that break the limit by several orders of magnitude. Enforce breaking them into smaller chunks. Either this is possible, or the change is not well designed by its author.
  • do not check libraries into your repository, but handle them through a build system like maven or scons. This is such a general recommendation, that it makes me wonder if there is a special constraint in your team to check libraries in; in that case, I would suggest to you keeping them in a separate repository. If they are not your code, then you do not have to review them.
  • plan and reserve developer time to do code reviews. This should definitely count as development time. At first, the process will slow you down, but in a few months it will keep you moving at constant speed.

I would discourage trying to find tools that automate code reviews. This defeats the two main goals of the review: have a human review the meaning of a change that would escape a machine, and spread the knowledge about the code base among the developers.

  • "Review smaller chunks of code" was also suggested in the comments. My fear is that by doing that I'm going from the frying pan to the fire. Let's say I review one big commit: there's 2 min of context-switching in to "review mode", then 40 minutes of reviewing code. Then instead let's say I review 2 smaller commits. That's 4 min of context-switching, 40 min of review, plus a few more minutes of review because I review some lines in commit 1 that get taken out in commit 2. That seems like more, not less time spent. Could you possibly explain what's wrong with my math/assumptions? – machineghost May 23 '14 at 17:14
  • And as for tools, I meant some sort of human-assistance code review tool, like Rietvald; I 100% agree automated code reviews can't replace human ones. – machineghost May 23 '14 at 17:15
  • You are right that code reviews are very time consuming. There is one thing wrong about your "context switching" assumptions, though: in the process of code review, you can ask the requester to do any necessary changes, and this means it would be strange for the same requester to modify the same lines again in a next merge request. You should try to put a fair share of the workload on the requester: the review is only over once a requirement has been met or a bug fixed, all tests pass, and all style conventions are met. This can be also a lot of work for the requester. – logc May 23 '14 at 17:48
  • As for tools, I am sorry to say I don't have any experience with tools other than diffs and plain conversation. Maybe such support tools are worth checking out! – logc May 23 '14 at 17:50
  • complexity ≈ conflicts² so for multiple commits/reviews we get complexity ≈ a² + b² + c² + … and for a single commit/review we get complexity ≈ (a+b+c+…)² and it can be seen that a² + b² + c² < (a+b+c)² – ctrl-alt-delor Dec 6 '14 at 15:17
4

Sometimes you can't get away with a small change. You might need to forgo GitHub's code review tools and use Plain Old Git:

$ git remote add somebody https://github.com/somebody/repo.git
$ git fetch somebody
$ git diff --name-only development somebody/topic_branch
# shows list of files that were touched
$ git diff --name-status development somebody/topic_branch
# Shows list of files plus whether they were added, deleted, modifed, renamed, etc.

Then you can use:

$ git difftool development somebody/topic_branch -- ./path/to/file/or/directory

This will help you whittle down the diff.

Or just see the diffs of all non added files:

$ git diff --name-status development somebody/topic_branch | grep -vE '^A'

If you have a comment to make, you can always head back to GitHub and comment on the file or line once you've had a chance to review things.

  • I'll take a look at using git difftool; Github is so convenient (especially since I can make comments right away as I read), but at the same time it'd also be convenient to eliminate the external libraries directory from the diff. – machineghost May 23 '14 at 16:46
3

A working solution is to put a soft upper limit on how large merges you accept, but changes have to be reviewed as some point.

If you don't have much experience with source code reviews yet, you could start out recording metrics of the reviews (too):

  • Merge/commit size
  • Issues resolved
  • Errors introduced
  • Review reports
    • Reviewer
    • Time used
    • Volume of comments
    • Conclusion
  • ...

These numbers will help you figure out what sizes of merges/commits are optimal in your organisation.

For your specific example, just committing the external library separately would have made the whole problem much, much smaller. - My basic assumption about merge/commit reviews is that the time it takes is proportional to the square of the size of the "diff" (I am at least certain that review time is not just linear, as you assume). If you collect the metrics, you will be able to see how wrong my assumption is.

  • If I had a performance bug in my code, I wouldn't start by trying to solve it, I'd profile first to find out for sure where the issue truly was. Since my question (like a performance bug) seems to have a variety of possible causes, I think it makes a lot of sense to narrow it down by collecting more information. Thanks for the suggestion. – machineghost May 23 '14 at 17:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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