Fixing Code Reviews

[David Edmundson]

One of the parts mentioned in the original document for setting up the KDE
quality team was fixing code reviews.

Code reviews are probably the most important step to improving quality and
so many projects appear to be simply skipping them - or only applying code
reviews to "outsiders", and this is a real problem.

I'm currently working my way looking through some piece of code in KDE (I
really don't want to name names here) and there's so many constant
"mistakes" that I would never have let through code review if I was doing
it.
(calling delete on qobjects, leaking objects, unclear APIs, badly named
slots, non-virtual destructors, and even a frickin' goto statement in a
section of code that created/deleted objects..really horrible stuff) but
it's not the author's fault if no-one is reviewing it pointing out what's
wrong.

We have mandatory reviews in KDE Telepathy, and it generally works really
well. Pretty much everything is reviewed, and I'm not saying our code is
perfect, but it's a damn load better than if we didn't have it. Reviews
give a chance for code that is perfectly good, to be made better, and we
argue a lot about UI/user facing text just as much as the code itself. I
wrote about some of the benefits on my blog
http://www.sharpley.org.uk/blog/code-reviews.

I don't know why other projects don't do it but I think it's because:
- They think it slows development
- There's no-one around to review (the only legit reason)
- Ego
- Laziness/They don't see reviews as useful.

The reason I think it works well for us, is because of the following:
- We're really fast at turning round reviews (generally ~30 minutes, up to
2 days max)
- We use pastebin + IRC / IM for trivial patches
- There are lots of us who do reviews. At least 6 of us are happy to say
"ship it!" on things, and a few more will participate in the review process.
- We all do it, there's no sense of "I'm pro, I get to skip reviews", which
means no-one else feels they can skip.
- If anyone does try to commit something without a review I stab them in
the face.

Conversely I have a patch on plasma that has sat there for a _fortnight_
not going anywhere, and frankly it makes me want to not bother with a
review next time. Also I work on another KDE project where I don't bother
with reviews, partly because it's in playground and partly because I don't
think anyone else would review any changes I did make.

*So how can we fix it?
*
It's a mentality change as much as anything else, I don't think going round
saying "you don't review, fix it!" will change anything.

The other problem is that in many cases that I've seen from lurking in IRC
channels and alike is that the project leads are setting a bad example. The
only fix can come from them changing first, which is a bit of a circular
problem because as a project-lead I think it's hardest to find someone
willing to review your code.

*Ideas:
*- create a way for a developer to say "hey, I need more people to review
code in my project". (they could email us?)

- email individual project leaders saying "hey, you don't seem to be doing
many code reviews. Why not? Is there anything that can be changed to make
that happen". Then get more aggressive.

- break the misnomer that you have to be an expert/know the code to do a
code review. Sure it helps, but some review is still better than none.
Blogs? Encourage GSOC mentors to make their students review some of the
mentor's own code?

I'm not really not sure how else to solve this...


David Edmundson
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-testing/attachments/20120413/136edfd8/attachment.html>