Discussion on code reviews from google plus

[Thomas Ferris Nicolaisen]

As you may remember, I forgot to mark this Google+ discussion as public, so only my Google+ followers were able to contribute in the comments. 

Here I've pasted in the whole thing with comments. Hope it works and is readable!

(I apologize to any commenters who didn't really want their comments out on a public mailing list, but seriously I don't see any secrets in the stuff written here, and as it was already shared with my "extended circles", it could hardly be considered not-public in the first place.)

------begin paste---------

Thomas Ferris Nicolaisen  -  Oct 16, 2011  -  

Tomorrow we're discussing Code Review at the Bonn Agile Meetup (http://bonnagile.blogspot.com/). So I asked today on twitter: "Warming up for tomorrow's #bonnagile discussion: In which contexts are code-reviews a good practice?"

+Bodil Stokke asked in reply: "In which contexts could code reviews possibly be bad practice? I guess that'd be a short discussion. :)"

Ole Christian Rynning added: "agreed.There are few bad contexts. However; there are several BAD practices and pitfalls you can fall into when reviewing:)
to name a few (opinionsted) criticism, syntactical, pedantery, wordfeuds, mistrust, direction over collaboration, ovnership, etc..."

+Johannes Brodwall said "I've heard lots of love for code review, but the few times I've seen it, it's been a frustrating waste of time. YMMV, I hope"

Then I asked on pairing:

Ole Christian Rynning said "well if you pair, you already have reviewed :)"

+Bodil Stokke "Review needs fresh eyes, ideally out-of-project eyes. Pairing as review is generally a bad idea in my experience. (As opposed to pairing with the reviewer, which is ++helpful.)"

Ole Christian Rynning "we normally review nonpaired features. Correctness, readability, etc. don't spend much time on design preferences (in revw)"

Bodil replied: "Yeah, review should be primarily about reducing wtfs/min, ie. readability and reduction of complexity"

+Knut Haugen also joined in later on, but I have to fly out the door now for a while. Maybe the discussion will be more easy to track on google+ than twitter :P

Twitter / T. Ferris Nicolaisen: Warming up for tomorrow's ...

  -    -  

Knut Haugen - I agree to turn the question around: in which contexts is it a bad practice? And when that runs out turn to code review anti-patterns and how to make it last. Things that come to mind is perhaps trying to track code metrics over time to see if the review is helpful, track programmers feelings about e review and do they actually feel it helps, etc. 

We are using reviews now, with a digital tool to aid us, creating diffs, commenting, notifying etc. Partly because of distributed teams. It is not mandatory and the participation is somewhat random and varies over time. Tips on how to make it a lasting practice are welcome. :-)

Oct 16, 2011   

Thomas Ferris Nicolaisen - I wrote "I've never worked at a place where there was regular reviews. Tried it out a few times, positive exp, but didn't stick to it"

+Knut Haugen asked "why didn't you stick with it? We are using reviews at work now, but is hard to make it stick and be 2nd nature"

My answer: Well, not sure really :) It was a bit of effort to arrange the review the first time (I had to select some bits of code to review). After the initial review, nobody asked to continue the practice, and I didn't drive it on myself..

Oct 16, 2011  -     

Knut Haugen - One of my theories is that it can be hard to actually see and quantify the results of a code review. You can change the code after a review, but did it actually help? And when not seeing a result, positive or negative, we ditch it. It is, at least, one theory. +1 on making this an open space at smidig

Oct 16, 2011   

Thomas Ferris Nicolaisen - But I'm not going to the Smidig conference :(

Oct 16, 2011  -     

Knut Haugen - For those of us who are (I'm not certain either, but hoping) :-)

Oct 16, 2011   

Thomas Ferris Nicolaisen - +Knut Haugen Regarding quantifying results: Yeah, it's hard to measure if it's having a good effect or not. I like to judge agile practices by asking what the team thinks about them in a retrospective, as there are few other ways to measure properly (I tend to think most bugfix-statistics are fake or warped to fit their purpose).

Oct 16, 2011  -     

Thomas Ferris Nicolaisen - Just to track more comments from twitter here:

I asked +Bodil Stokke "Ref: Pairing as review: how does it work and how is it bad?"

She answered:

"The idea is that if somebody helps you write the code, they're reviewing as you go. Never seen that turn out well.

Review is often a workflow phase in whatever issue tracker the team is using. Resolve -> review -> close. 

Or, better yet, it's integrated into the VCS workflow: maintainer reviews pull requests before merging them."

Oct 16, 2011  -     

Thomas Ferris Nicolaisen - +Bodil Stokke My reply: Ah, I see. The current models I have for code review techniques are roughly these:

0) Pairing? (not fresh eyes) Not really considered code review for our discussion.
1) Pre-commit, (Eclipse Synchronize view?) Reviewer studies the diff before the developer commits.
2) Commit mails (not interactive, already checked in) - Reviewers study commits, after they are committed.
3) Commits/patches are shared and reviewed as pull requests before being merged into mainline.

PS: I'm demonstrating (3) tomorrow by using the code review tool Gerrit (http://code.google.com/p/gerrit/).

Oct 16, 2011 (edited)  -     

Bodil Stokke - My list matches yours, except I'd call #1 pair programming with the reviewer (which is my favourite, but doing it pre-commit may be impractical), and we'd do #2 by tagging commits with Jira issue IDs and relying on Jira to tell us which commits that should be reviewed - that way you can review the whole issue rather than individual commits, which tends to be more sensible.

As for #3, I've tried both Gerrit and Github, and must admit to a massive preference for the latter...

Oct 16, 2011   

Thomas Ferris Nicolaisen - Yeah, it's hard to beat Github. But Github costs money :) Gerrit always treats single commits as pull requests, which kinda forces its way into your workflow, making it very strict. And the GUI is usability-wise horrible.

Oct 16, 2011  -     

Thomas Ferris Nicolaisen - +Bodil Stokke Isn't it a bit annoying that you always need to create a Jira issue in order to get something reviewed though?

Oct 16, 2011  -     

Erlend Oftedal - I really see the benefit of code review, but I do think it should be done in pair (dev + reviewer) to be most successful and useful, and that the reviewer and dev should work together to fix whatever they find there and then. Less context switching, less disconnect etc.

Code review is useful because even though we aim to spread knowledge within a team, people will still have different expertise, and thus may look for different things. A ORM expert may look for other things than a security expert etc.

Oct 16, 2011   

Thomas Ferris Nicolaisen - +Erlend Oftedal How do you initiate a review? Is it a fixed procedure or on-demand?

Oct 16, 2011  -     

Thomas Ferris Nicolaisen - And how do you select who will review what?

Oct 16, 2011  -     

Erlend Oftedal - It may also make sense to incrementally build a minimal code review guideline, which mentions touchpoints. If it's too detailed and becomes a checklist, it may become the review and thus anything not in the list, will not be checked.

Oct 16, 2011   

Erlend Oftedal - +Thomas Ferris Nicolaisen We have a code review column on the white board (we run kanban). So a task is not done, untill reviewed. Find a reviewer during standup.

Oct 16, 2011   

Thomas Ferris Nicolaisen - +Erlend Oftedal Aaah, interesting. That sounds like a great pattern for assigning code review. I think that doing that process in Jira (or any other electronical task tracker) sounds rather tricky, as you'll probably need some default assignee who's responsible for re-assigning all the review tasks.. +Bodil Stokke how do you do that part?

Oct 16, 2011  -     

Bodil Stokke - On my current team, keeping the issue tracker tidy is a job for the designated Jira monkey, ie. the scrum master. Although if these things become unmanageable without an issue janitor, that's probably a sign that your team is too large and/or your iterations are too long. You should be able to look at the task board and know which issues need code review and whether you're qualified to perform it.

Oct 16, 2011   

Thomas Ferris Nicolaisen - +Bodil Stokke +Erlend Oftedal You both have one individual reviewer per task? Edit: One reviewer reviewing him/herself, or is it done in co-location with the committer?

Oct 16, 2011 (edited)  -     

Erlend Oftedal - The committer puts the task in the ready for review column. Someone else takes it, but very often does the review in pair with the committer. At least the committer explains what have been done, what parts have been touched and why. Which files have been modified can of course be found in the source control system. Pairing is better because of knowledge transfer and less context switching due to the fact the a lot of things can be fixed when found.

Oct 16, 2011    

+1

   

Bodil Stokke - +1 to that. It also helps keep the reviewer from indulging in any excesses of restructuring or refactoring. :)

Oct 16, 2011   

Knut Haugen - We have a rule on that too: reviewer does not change anything, but ask questions, suggests and discusses. If anything is to be changed, it is the author who should do it.

Oct 16, 2011    

+1

   

Johannes Brodwall - As an (infrequent) reviewer, I usually feel that the most pressing issues will be too large to fix. I've never got the feeling that the author learn to be a better programmer through a code review, so the next review will be as bad. In my experience, the learning is so ridiciously slow with review compared to pair programming that it's not even a point.

Oct 16, 2011   

Thomas Ferris Nicolaisen - In my first code-review, we sat the whole team (5 people) together in front of a projector with our code basis. We then studied how we were doing Struts actions (I had picked out a couple of really nasty cases up front). I think the goal was to weed out the worst inconsistencies, have some discussion on code-style/patterns. The outcome was some agreements, and we all agreed it was a pretty interesting session. We didn't do more later on, probably because we were getting too busy, and were happy enough with the code that was coming in from the others.

The next code-review I did (a couple of years later), was similar: I ran FindBugs over the code, picked out the worst 10 examples, and talked with the whole team (this time 15 people) how and why we could/would deal with problems like that. The bugs were so horrible, and nobody could remember who had authored them mostly, so it was quite a funny and well-spirited event. We had a little discussion on exception handling, agreed to use FindBugs in our CI, and again, didn't keep up any sort of code review meetings in the future.

One might argue that neither of these events were proper code review... But this whole-team-studying-and-discussing-code-together thing was kinda how I always imagined code-review being like, perhaps not the whole team all the time, but the goal being to reach consensus in the team on how code should be.

Oct 16, 2011  -      

+1

   

Johannes Brodwall - I've done the same code "show and tell" as you, Thomas, with reasonably good results. I don't consider it Code Review (tm), though.

Yesterday 12:00 AM    

+1

   

Erlend Oftedal - No disagreement here. I also think pair programming is way better.

Yesterday 10:07 PM   

Thommy Bommen - I want in on this discussion. I force a code review before check-in on the projects where i am project lead. I do this for two things. Quality of the code it self and I do it because I need someone else than the committer to know at least some of the code. It is all down to asking the right questions. I have delivered a suggestion for a roleplay lightning talk to smidig 2011 on it. I've had this roleplay done in a project with interesting results.

When working with something that has a deadline we tend to imagine things. One of those things is the idea that the project lead do not want you to use time on code review and deliver good quality if it takes more time. I've done code reviews of 15 minutes with changes to the code which improved it. to me that can not be a waste of time.

Another example is when someone in our project became sick and had to stay home for 3 days. The other guy that had been reviewing his code could actually step in to do the last changes to make it ready for demo! In my opinion this has to be done in any serious project with different level of experienced people.

12:24 AM   

Thomas Ferris Nicolaisen - +Thommy Bommen I would love to hear more about how you actually review? How is the rhythm/workflow around a code review? What triggers a review, and who does it?

12:14 PM  -