About the address book – I’ve received a bunch of email making suggestions and asking for things. That’s great! I’ll comment on that shortly – I just need a little more time to clear Account Provisioner and Tabs on Top off my plate.
Well, if there’s one thing that my supervisor Greg has taught me, it’s how I shouldn’t rush headlong into something before all of the facts are in. So before I decide to do something like game-ifize code review, I should take a look at some prior work in the area…
In particular, check out the following slide-show. Flip through it if you have the time. If you don’t have the time, scroll down, where I get to the salient point with respect to game-ificating code review.
Without play, there is no game. Points do not equal a game. It’s not nearly that simple.
Free Pizza and Pop
I’m going to divert for a second here.
Last week, a company set themselves up a couple of booths in the lobby of the Bahen Center where I work. They were there to recruit university students to work for their company – either as interns, or full-timers.
They were also handing out free pizza and pop.
Needless to say, I wanted a few slices – but I figured it would be polite if I engaged them in conversation before waltzing off with some of the free food and drink they’d brought.
So I sparked up a conversation with one of the recruiters, and he told me about the company. I’m going to call this recruiter Vlad.
I ended up gently steering the conversation towards code review, and I asked my inevitable question:
“So, do you guys do code review?”
I felt like a dentist asking a patient if he’s been flossing. Vlad waffled a bit, but the general impression was:
“Not as much as we should. We don’t have a prescribed workflow. It’d be hard to persuade all of the teams to do it.”
And then we started talking about code review in general. It turns out that Vlad had worked in a few companies where they’d done code review, and he always felt a little short changed. He said something along the lines of:
“I never felt compelled to do reviews. They just sort of happened…and I did it, and it felt like…unrecognized effort. I mean, what’s the incentive? Do you know what I mean? There’s incentive for the software, but I’m talking incentive for me. And some people did really lousy reviews…but my reviews were treated the same as theirs. I didn’t get recognized, and didn’t get rewarded if I did a good review. So it was hard for me to do them. I want to be recognized for my good reviews, for my good contributions.”
I wish I’d had a tape-recorder running so I could have gotten Vlad’s exact words. But that’s what I remember him saying.
Feedback and Recognition
Maybe instead of trying to game-ulize code review, I can instead hear what Vlad is saying and work off of that.
With the code review that Vlad participated in, all of the feedback went to the code author, and none went to the reviewers. And the reviewers are the ones who are doing all of the heavy lifting! As a reviewer, Vlad also wants feedback, and recognition for code review done well.
There’s a company in Toronto that specializes in feedback like this. They’re one of the major players in the Toronto start-up scene, and have built a pretty sweet suite of tools to facilitate quick and easy feedback/recognition.
The company is called Rypple. And maybe that’s the name of the application, too. (checks website) Yeah, it’s both.
So Rypple has this feature called Kudos that let’s people publicly acknowledge the good work of their team.
Normally, I don’t pimp companies. And it upsets me when people comment on my blog, and their sub-text is to try to sell their product or service. However, I think this video is relevant, so I’m posting their demo video so you can see how Kudos work:
So Rypple’s idea is to have a feed that the team subscribes to, and publicly display things like Kudos. The badges for the Kudos are also limited in how many you can give per week, so they’re a valuable commodity that can’t just be handed out all over the place. Cool idea.
So there’s one approach – use a service like Rypple to give your reviewers better feedback and recognition.
Or maybe we could build an extension for Review Board that does something similar, and more oriented around code review.
It’s not oriented like a game, like I had originally envisioned. But somehow, I think this idea has more meaning and traction than just “adding points”.
More on this idea in a few days. But please, comment if you have any thoughts or ideas to add.
From my software development experience, there are a few different words for the generic notion of a “problem”.
A bug or a defect, for example, is defined as the following from Wikipedia:
A software bug is the common term used to describe an error flaw, mistake, failure, or fault in a computer program or system that produces an incorrect or unexpected result, or causes it to behave in unintended ways. Most bugs arise from mistakes and errors made by people in either a program’s source code or its design, and a few are caused by compilers producing incorrect code.
An issue goes a level higher – a bug is an issue, but an issue might not be a bug. Wikipedia says:
In computing, the term issue is a unit of work to accomplish an improvement in a system. An issue could be a bug, a requested feature, task, missing documentation, and so forth. The word “issue” is popularly misused in lieu of “problem.” This usage is probably related.
Anyhow, the long and the short of it is – we’re going to be calling “problems still existing within a review request revision” issues. And this is distinct from the sort of thing that might show up in an issue tracker.
Maybe down the line, we’ll have a way for administrators to set their own word for it. From the thread, it sounds like everybody and their brother has their own favourite terminology. ”Issue” will have to do for now.
Thanks again to everyone on the list who contributed to the conversation.
However, since we’re adding this to the core of Review Board, we have to keep it simple. One of Review Board’s biggest strengths is in its total lack of clutter. No bells. No whistles. Just the things you need to get the job done. Let the extensions bring the bells and whistles.
So that means creating a bare-bones defect-tracking mechanism and UI, and leaving it open for extension. Because who knows, maybe therearesomepeople out there who want to customize what kind of defects they’re filing.
I’ve come up with a design that I think is pretty simple and clean. And it doesn’t rock the boat – if you’re not interested in filing defects, your Review Board experience stays the same.
Filing a Defect
I propose adding a simple checkbox to the comment dialog to indicate that this comment files a defect, like so:
No bells. No whistles. Just a simple little checkbox.
While I’m in there, I’ll try to toss in some hooks so that extension developers can add more fields – for example, the classification or the priority of the defect. By default, however, it’s just a bare-bones little checkbox.
So far, so good. You’ve filed a defect. Maybe this is how it’ll look like in the in-line comment viewer:
A defect has been reported!
A reviewer can file defects reports, and the reviewee is able to act on them.
Lets say I’m the reviewee. I’ve just gotten a review, and I’ve got my editor / IDE with my patch waiting in the background. I see a few defect reports have been filed. For the ones I completely agree with, I fix them in my editor, and then go back to Review Board and mark them as Fixed.
It’s also possible that I might not agree with one or more of the defect reports. In this case, I’ll reply to the comment to argue my case. I might also mark the defect report as Pass, which means, “I’ve seen it, but I think I’ll pass on that”.
I think I'll pass on that, thanks.
These comments and defect reports are also visible in the review request details page:
A defect has been filed.
All fixed up.
It's all good - just pass this defect report.
What do you think? Am I on the right track? Am I missing a case? Does “pass” make sense? Will this be useful? I’d love to hear your thoughts.
I just spent the long weekend in Ottawa and Québec City with my parents and my girlfriend Em.
During the long drive back to Toronto from Québec City, I had plenty of time to think about my GSoC project, and where I want to go with it once GSoC is done.
Here’s what I came up with.
Detach Reviewing Time from Statistics
I think it’s a safe assumption that my reviewing-time extension isn’t going to be the only one to generate useful statistical data.
So why not give extension developers an easy mechanism to display statistical data for their extension?
First, I’m going to extract the reviewing-time recording portion of the extension. Then, RB-Stats (or whatever I end up calling it), will introduce it’s own set of hooks for other extensions to register with. This way, if users want some stats, there will be one place to go to get them. And if an extension developer wants to make some statistics available, a lot of the hard work will already be done for them.
And if an extension has the capability of combining its data with another extensions data to create a new statistic, we’ll let RB-Stats manage all of that business.
The reviewing-time feature of RB-Stats will become an extension on its own, and register its data with RB-Stats. Once RB-Stats and Stopwatch are done, we should be feature equivalent with my demo.
I kind of breezed past this in my demo, but I’m interested in displaying “review karma”. Review karma is the reviews/review-requests ratio.
But I’m not sure karma is the right word. It suggests that a low ratio (many review requests, few reviews) is a bad thing. I’m not so sure that’s true.
Still, I wonder what the impact will be to display review karma? Not just in the RB-Stats statistics view, but next to user names? Will there be an impact on review activity when we display this “reputation” value?
This is a big one.
Most code review tools allow reviewers to register “defects”, “todos” or “problems” with the code up for review. This makes it easier for reviewees to keep track of things to fix, and things that have already been taken care of. It’s also useful in that it helps generate interesting statistics like defect density and defect detection rate (assuming Stopwatch is installed and enabled).
I’m going to tackle this extension as soon as RB-Stats, Stopwatch and Karma are done. At this point, I’m quite confident that the current extension framework can more or less handle this.
Got any more ideas for me? Or maybe an extension wish-list? Let me know.
From the very beginning, my GSoC project has been mainly focused towards one primary goal: I want to build an extension for Review Board that will allow me to collect information about how long reviewers actually spend reviewing code.
That’s easier said than done. When I started, the Review Board extension framework wasn’t really in a state to allow such an extension to exist.
So I’ve been tooling around in the Review Board code for the past 2 months, preparing the framework, and getting it ready to handle my extension.
And last night, it started to work. I can now give rough estimates on how long a reviewer has spent reviewing code.
How It Works
My extension adds a new table to the database which stores “reviewing sessions”. Each reviewing session is associated to a particular review request and user, and also has a field to store the number of seconds that a user has spent in review.
The server receives this activity notification through the Web API, and checks to see if the time lapsed since the last session update was greater than 10 seconds. If it is, we increment the working session by 10 seconds and return a 200 HTTP code. If it isn’t, we don’t change anything and return a 304 HTTP code.
Next, my extension waits for a user to publish a review. When it notices that a review is being published, it finds the working session for that user and review request, and then attaches it to the published review. If the user then starts looking at the diff or screenshots again, a new working session is created.
The result? A pretty decent estimate of how long a user has spent reviewing the code. No time gets recorded if the user gets up and has a sandwich. No time gets recorded if the user is on another tab reading Reddit.
Not bad. For a first draft, anyhow.
I think I’m going to try to chart the data somehow, so that users can track their inspection rates. I’ll let you know how that goes.
I can’t honestly say I’ve been doing much test-driven development on Review Board. Django is a really cool web-framework, but I miss all of the nice testing tools from the Rails ecosystem.
Anyhow, today, I decided to run the tests, and BAM – Segmentation Fault:
Testing Perforce binary diff parsing … SKIP: perforce/p4python is not installed
Testing PerforceTool.get_changeset … SKIP: perforce/p4python is not installed
Testing Perforce empty and normal diff parsing … SKIP: perforce/p4python is not installed
Testing Perforce empty diff parsing … SKIP: perforce/p4python is not installed
Testing PerforceTool.get_file … SKIP: perforce/p4python is not installed
Testing parsing SVN diff with binary file … ok
Testing SVNTool.get_file … Segmentation Fault
Yikes. Getting a segfault in Python is unusual, and a little jarring. However, it did let me narrow down the problem a bit: it must have to do with the pysvn bindings that Review Board uses to talk to Subversion, because those are written in C++ (which can certainly segfault).
The machine I was using had Ubuntu Hardy on it, and the Hardy packages have pysvn 1.5.2-1. Taking a look at the pysvn homepage, the most recent version is actually 1.7.2.