Fixing Code Reviews

[Martin Gräßlin]

On Friday 13 April 2012 13:32:13 David Edmundson wrote:
> 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.
please raise awareness of it. E.g. create a bug report or contact the mailing 
list or best just open a review request for the issues you find.

Btw as I maintain one of the applications which has most of the things you 
point out: for some things it's valid and correct. E.g. there might be very 
good reasons to delete qobjects.

Things like non-virtual destructors could be handled by a Krazy check, so 
that's an easy way to improve (of course requires that devs actually look at 
these checks which I rather doubt).
> 
> 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.
+1 - that was a good post :-)
> 
> 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.
- Reviewboard sucks
> 
> 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.
You are lucky to be such a large team. With about three developers it becomes 
difficult to perform code review and be fast in development. My solution to it 
is to gather reviews and merge on (mostly) Sunday. The nice side effect of 
such an approach is that I am forced to use git the way it is meant to be :-)
> 
> 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.
yes that's true.
> 
> 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...
I think your first two ideas need to be combined into one. Create a group 
which is willing to perform code review on any random project. Of course they 
cannot give a good review as they don't know the source code and don't know 
the special requirements.

If such a team is established all project leads are contacted to inform them 
about this service and we offer them to review their code. This could break 
the circular problem you wrote about. For example I really dislike that I very 
often have to push changes which have stayed in review for a week without any 
review. That's the disadvantage of being a project leader :-(

Once such a workflow is established we can go and push the projects which are 
not participating.

A step afterwards would be to go to the module coordinators in case of KDE SC 
projects and lock down the stable branches to only allow commits which have 
either a REVIEW or reviewed-by tag. It would be a drastic change in the way we 
currently allow access to our repositories, but I think it is highly required 
to have a better quality of our stable branches which do not get tested at 
all.

Cheers
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-testing/attachments/20120413/91be4f50/attachment.sig>