I wanted to leave a comment, but "you must be logged in". So, I'll put them here.
- Why does your code review take so long? Should it?
- Pair programming, though very effective and valuable, is difficult to sell or get working properly... but why can't review be a rotating thing. Spread the love, normalize your review approach.
- There are several comments in the post skirting the testing topic, but nothing blatantly stating it. Tests should pass first, then check in. If the tests pass, then any refactoring caused by the code review should be easier and lower risk. This safety net will allow you to check in first and then do the code review second.
Kevin, thanks for the comments, I've adjusted my blog to allow anonymous comments.
ReplyDeleteIt is certainly a shame that our code review seems to take a long time. I think there are several reasons:
1. Reviewer's scope is not clear - we don't have a clearly defined aim for our code reviews so some people get extremely detailed and inspect it to the nth degree, taking quite a long time.
2. Reviews aren't given priority - Our process says that code must be reviewed but it doesn't really say who should decide on the reviewer and whether it should be prioritised, some reviewers will be heavily loaded, or have their own critical work that they choose to prioritise.
3. Amount of code being reviewed - Often a code review is all the changes made in an interation in an individual branch, this can sometimes equate to 4 weeks work by 3 people = 12 man weeks of work!. Perhaps we should be merging more often, or at least splitting up the code to be reviewed between more reviewers.
Our latest issues with code review and merging is making us question why code reviews often take so long.
We do tend to rotate the reviews so each code area will be reviewed by developers in the same team, developers in other teams and architects.
Unfortunately we don't have a full test suite, and different team members write tests to varying degrees. The tests we do have are run on check-in. This point was certainly one I used to argue for the 'review after' approach.
One blocker we have is the generally accepted approach to the development process, that I personally think just doesn't work; Check-in to main has always been seen as the be-all-and-end-all, meaning that no further work needs to be done. This has come from the historical approach, and although we are changing this idea still lingers. Once code is in Main, management and development move focus to new areas of work and the code review would be neglected and probably never happen. I think it's a shame that we can identify that this would be an issue, agree that it's a bad thing, but somehow not be able to prevent it happening.
Wow... lots of conversation... thanks for the response, I'll take my thoughts over to fluxmunki's post (link above) since the debate has taken a good run there.
ReplyDelete