When I first started hacking on KDE-Telepathy the original manager was very insistent that all code should be reviewed before merging. Initially it was something that I considered a complete waste of time but has since grown on me as a really important step in development.
Recently, I've been getting involved with following another very prominent important KDE project and I've been a bit concerned by the "hack and slash" attitude that I see in that IRC channel with commits coming from all over the place, with very few of them looking like they're actually reviewed.
What's good about code reviews
Code reviews obviously result in "better code", but I think there is a tendency to think it's only for people making their first patches. This attitude is utter nonsense.
Many eyes make all bugs shallow
Also known as "Linus's law", the more people who look at a piece of code the more likely you are to find a bug. It's a lot easier to fix a potential bug at review stage than find it after it's deployed. Everyone makes mistakes, it's the best time to find them.
Shared responsibility
This is tied into the above. Let's say Martin submits a patch which I review which turns out to be crashy. Who's fault is it? It's his fault for the dodgy code but it's just as much my fault for not spotting it. It's also everyone else's fault for being too lazy to review the patch. I think code reviews take away some of the "blame game" when it comes to bugs.
Code habits
This is more for the people starting out, but it's a good time to spot the bad habits of missing "consts", bad whitespace, not checking for null pointers etc. It's better to point these out here and "block the commit" publicly so the submitter learns than to simply fix it for them.
Usability issues
One of the main things we've gained from reviewboard is fixing situations where the code is perfectly fine, but what it's doing isn't. At review is a good time to make sure that not only the code is correct but that the user experience is the best it can be. This means checking any strings presented to the user, we've had huge arguments over the tiniest of details "Alias" vs "Nickname" and checking all forms are visually well laid out and arguing over each pixel in a paintEvent. It can be frustrating at the time, but I reckon we get a better product for it.
Attaching screenshots help. Occasionally we've done UI testing just with a screenshot. I've sent someone else a link and said "explain to me what you think this button does". If they're unsure, the text/layout needs changing.
Keeping code simple
A good coder can write complex code, an expert coder can achieve the same complex tasks in a really simple way. It seems counter-intuitive but the better programmer will be writing simpler code as you learn how to design things better. I have rejected perfectly working patches saying "rewrite this part so it's easier to read" or even a simple "comment this". If it takes me several minutes to understand a patch at review time where I'm only looking a small section and have a description it's going to be a real pain for the next coder who wants to edit some code around this or fix it.
I've heard rumours of code in KMail with a comment that says "//Don't touch this unless you are ....". That's a horrific state to be in, and a lesson we should all learn from to avoid. It's far easier to sort this at review time when the author can remember how the code works.
A second set of eyes may be able to find a cleaner way to do it, it's often easier to find the best way to solve a problem once you've first drafted something.
More awareness into goings on in the project
If I have to read the code well enough to be able to review it properly I'll have a better understanding of how everyone else's code works, this is very useful when you end up having to add a feature somewhere that relates to their code.
What sucks, and how to avoid it
ReviewBoard is sooo slow to use
Use post-review, It does work. However the version shipped by your distro may be too old for the version of reviewboard we have. Follow the instructions to installing post-review on the reviewboard website.
It's still slow
The post-review flags make things a lot easier, --guess-description is pretty good, it fills everything in based on your git commit logs.
Make an alias for reviews you type often. This is mine for all KDE Telepathy reviews.
alias tpreview='post-review --parent=master --tracking-branch=anonupstream/master --guess-summary --guess-description --target-groups=telepathy -o
It's still really slow
On #kde-telepathy you'll constantly see links to pastebin being sent about. We use this for small patches, it's not as formal as using review board but it gets the same job done.
Another speed-up trick we do is to point out changes but say "ship it!" anyway. If only trivial fixes are needed, there's not always a lot of point making them submit a new patch with it fixed.
Pushing a git branch is easier than reviewboard
Use that then, the tool isn't important, what's important is making sure reviews happen.
It's pointless for small patches
As with anything common sense is needed. I once got bugged on IRC "d_ed, d_ed can you review this" and showed me a change from "#include <QLabel>" to "#include <QtGui/QLabel>". Be pragmatic.
It blocks development
No it doesn't. As soon as you submit a review, create a new branch, carry on working. If you think reviews block development it probably means you need to improve your git-fu.
It's disheartening to see your code ripped apart
Absolutely, I once had a 2000 line patch that I wrote on Christmas Day torn apart by Martin who proved part of it was pointless and cut it down to about 400 lines, removing this really clever feature I had. It's better code for it though. Deal with it.
Reviews sit there for ages untouched
This is the biggest issue we face, some difficult patches no-one wants to review, and in such a small team like ours people are often simply away/busy. Having a patch stagnate is frustrating for everyone.
I also found a lot of the new people in the project felt "unworthy" to do reviews, which is silly. If I submit a difficult patch I may be waiting for a specific person to review it, but that doesn't mean any other reviews aren't highly valuable.
I'm not sure of other good ways to solve this.
It's hard to review large amounts of pre-existing code
Yeah 🙁 I have no idea how to solve this. We got this even for the few small apps in KDE TP, as every new app starts off as "just prototype code" in some scratch repo somewhere, then end up getting merged.
Summary
- Everyone should have their code reviewed. If you think you're so good that you don't need reviews, you're mistaken.
- Reviews are the perfect place to discuss UI isssues. Think about and challenge every user facing word, every UI decision.
- Make that easier, attach screenshots
- Use the appropriate tools for the job, for us this isn't always reviewboard for small commits.
- Everyone should be involved in reviewing, don't leave it for someone else or people will be put off submitting things for review.