So, for my Master’s thesis, I’ve pretty much set my heart on code review as my research area. Here’s why: it works. It makes your code better. It helps you find bugs. And I’m not just quoting something overheard in a pub – there’s evidence to back these claims up.
And then there’s my own experience to boot – the MarkUs team has been using ReviewBoard as our pre-commit code review tool since last summer, and I wouldn’t ever go back. If I ever have to work in a shop that doesn’t perform code reviews, I’ll campaign my butt off.
Having said all that, pre-commit reviews certainly aren’t for everyone. Some downsides of pre-commit over post-commit:
“The major downside of pre-checkin code review is that it puts a major bottle neck on getting changes into the system for other developers to integrate with early enough.” (from this link)
For some applications, testing takes hours on end. Why wait? Might as well toss it into the repo, let the Continuous Integration build it, and just see what happens.
There are probably more.
My response: at least for MarkUs, pre-commit code reviews are working just fine, thank you very much. At least we’re reviewing it – and any review is better than no review. But to continue my response, here are a couple of advantages of pre-commit code review for the MarkUs development team:
Since most students working on MarkUs are doing it for half-credits, this means there’s a lot of turnover every semester. ReviewBoard lowers the chance of our new hotshot developers accidentally slipping something ridiculous into the repository and having to do that neat Subversion trick of pulling it out again. This is the obvious one.
It helps all developers keep track of what everyone else is doing. This is true for post-commit reviews too, but it’s certainly worth the mention. It sure beats reading SVN log messages…
It’s a great arena for new developers to ask questions. Our new developers this semester have been very active on our ReviewBoard, asking plenty of questions about things that are showing up in the diffs under review. Sometimes, “theoretical” code is posted to demonstrate how something would be done. Post-commit does not support this nicely.
It’s an excellent way of showing how you’re coming along with a task, without the embarrassment of breaking the build. MarkUs developers sometimes post up “sneak previews” just to give everybody a taste of how their particular task is coming. This “sneak preview” gives the opportunity for other developers to critique the direction that the submitter is going in, and offer pointers in case they seem to be heading off in a hazardous direction.
Yep, there’s just something so satisfying about seeing all of those little green “ship-it’s”, and then firing off your code into the repository… it’s positive reinforcement for code reviews. And it’s strangely addictive to me.
Another Idea to Augment This Process
A little while ago, I wrote about what I consider to be one of the Achilles’ Heels of Peer Code Review. Here’s another one: at least for ReviewBoard, during a review all you’re looking at is the code. That’s all fine and dandy if you want to look at the logic of the code…but what if you want to try it? Does trying it out help find more bugs than just looking at it?
Well, at least for MarkUs, it’s helped. I’ve recently started checking out a fresh copy of MarkUs every time a review request is put up, and splat a copy of the diff under review on top. I run the test suites, and if they pass, I drive it around. I try out the new features that the diff supposedly adds, or try to recreate the bug that the diff supposedly fixes.
And I’ve caught a few bugs this way. This is because ReviewBoard is good at showing me what is in the code, but is bad at telling me what is not there. And that’s perfectly understandable – it’s not psychic.
So here’s an idea – how about writing a little script that checks ReviewBoard for new review requests. When it finds one, it checks out a brand new copy of MarkUs, splats down the diff under review over top, runs the tests, and then posts back as a ReviewBoard reviewer how many tests passed, how many failed, etc. If we wanted to get fancy, the script could even do some commenting on the code – maybe using Roodi, Flog, Flay, or some of those other sadistically named Ruby tools to say things about the diff. The script would be another reviewer.
And then the kicker – the script posts a link in its review where developers can try out a running instance of MarkUs with the applied diff.
Want a fancy name to kick around the office? Call it pre-commit continuous integration. I just checked – it’s not a common term, but I’m not the first to use it. Again, so much for being cutting edge.
Would this be useful? It’s possible that the Roodi/Flog/Flay stuff would bring too much noise to the review process – that’s something to toy with later. But what about the link to the running instance? Will that little feature help catch more bugs in MarkUs? How about for Basie?
Belated happy holidays! My last post was over a month ago, and so my blog has a nice layer of web-dust on it right now. Well, here I am to ease your mind. I’m still alive!
But that almost wasn’t true.
I won’t bore you with the details – I’ll just give you the facts, and let you fill in the blanks.
My girlfriend Em, her sister Cassie, and myself, were up in Collingwood on New Years Day, enjoying a relaxing day at a Norwegian spa (the outdoor baths were amazing – how awesome is it to be in a boiling hot tub, while simultaneously, your hair is so frozen that it’s snapping off in your hands?)
The roads that night were treacherous. Snowy, un-plowed, and dark. I had borrowed my Mom’s car for the trip, and we took it realllllly slow.
After a tortoise-paced two hour ride back to Em’s place in Newmarket, and then another two hour drive from Newmarket to my home in Grimsby the next day, I was getting pretty sick of winter driving. On top of that, the brakes seemed to be acting funny. I found myself sliding a lot, and there didn’t seem to be a lot of resistance when I put my foot down.
The next day, my Mom takes the car to go to work. She doesn’t even leave the drive-way. The brakes hadn’t been acting funny: the brakes hadn’t been acting at all. Turns out we had a leaky brake-line for the entire trip…
Guts of the story: I think we drove home from Collingwood with about 35% brake power in one of the worst snow storms I’ve ever driven in.
Breakfast tasted especially good for us that morning.
Anyhow, now where was I? Oh yeah…
MarkUs
MarkUs 0.6 got kicked out a week or so ago. The MarkUs Team kicked the crap out of a bunch of tickets over the holidays, and I think we ended up with a pretty solid release. MarkUs is being used again at UofT this semester, and Byron Weber Becker is also piloting it at UWaterloo. I’ll cautiously say that things seem to be going well for this release. Great job, MarkUs Team!
I’m TAing the students working on MarkUs for Greg’sUCOSP course again. We had a fantastic code-sprint this past weekend! The new team members have already started working on tickets and submitting code to review. I think we’re on our way into another highly productive semester.
This is a pretty dead-simple code review tool that came about during a Rails Rumble a few months back. It has that “big friendly buttons and round corners” web 2.0 thingy going on. I haven’t gone so far as to actually try it out, but I did watch this web-cast:
Not bad if you just want to get your code out there, and get your team commenting on your changes…
A few things caught my attention:
It’s a web service, so you don’t install it…you sign up for it
It currently only supports Git.
There doesn’t seem to be any support for contextual per-line commenting…I think it’s just file by file commenting. I’d love it if I could comment on a single line of code…
Still, if I was working on a project hosted on a Git repo, and I needed a dead-simple code review service, and I needed it quickly, I could probably do a lot worse than this.
Remember that time when I wrote about how it might be neat if somebody created a code review tool on top of Google Wave? (or Bespin for that matter – though I didn’t mention it, and should have)
Anyhow, looks like something Wave-ish (yet simpler, more streamlined) has been developed. Check out Squad.
I just tried this thing out for free (with ads, features locked, etc), and it was pretty cool. I could see something like this being very useful for showing new MarkUs team members how to do things. Actually, I just used it to show a new member of the MarkUs team how to use Shoulda. Pretty useful. It sure beats coding through IRC and Pastie.org.
A few things to keep in mind:
Super simple to get going – open up a session, and send someone a generated link, and you’re both coding in no time
One person codes at a time…so while one person edits, the screen is locked for everyone else
Ads on the left are a little annoying
Sports syntax highlighting for a number of languages – though I noticed that Ruby wasn’t one of them. :/
I can see this becoming second nature, like Pastie.org.
Who knows – I might find more reasons to use Squad as the semester rolls, and MarkUs picks up speed. I’ll keep you posted.
This service crowd-sources code review requests, so don’t expect to get deep architectural feedback, because it’ll probably come from strangers who don’t/barely know your code base.
The idea is – slap a piece of code that you’d like refactored up on the site, and then others swoop in with brilliant suggestions (assuming of course, you asked your question properly…check this out…what the…?)
This is the sort of thing that CS instructors probably wouldn’t want their students using too much…it’d then become solve-my-CS-programming-assignment.com.
Still, I think it counts as peer code review. And it’s way different that anything else I’ve been looking at. Nice.
If you haven’t already heard about Google Wave, watch this video.
In a nutshell, Google Wave is an attempt at integrating wikis, instant messaging, email and social networking, into a nice, tight, Google-y ball of goodness.
What’s particularly interesting, is that developers can write “gadgets” by coding against the Google Wave API. Check this out – a collaborative tool for systems modeling built into the Google Wave client. That’s pretty cool!
Which makes me wonder… could a code review tool be built on top of Google Wave? And if so, what services, if any, does the Wave protocol offer that might make such an application superior to other web-based code review tools?
What does Google Wave offer that would make such a code review tool special, more effective, and better?
Two more ideas to augment the code review process:
Screen Age
Imagine you’re reviewing a piece of code. There’s something like…500 lines to look at. So you’re scrolling through the diff, checking things here, checking things there…
And in the background, the review software is recording which sections of the code you’re looking at.
Hm.
I wonder if it’d be a useful metric to know what portions of the code were being looked at the most during a review? I believe it’s possible (at least within Firefox) to determine the scroll position of a window. I believe it’s also possible to determine the dimensions of the entire diff page, and the offset coordinates of elements on that page.
So shouldn’t it be possible to determine which elements were on the screen, and for how long?
Imagine seeing a “heat map” over the diff, showing where the reviewer spent most of their time looking.
Jason Cohen wrote that code reviews should take at least 5 minutes, and at most 60-90 minutes. I wonder if collecting this information would help determine whether or not a review was performed carefully enough.
Now, granted, there are plenty of ways to add noise to the data. If I’m doing a review, and stand up to go get a sandwich, and then my neighbour visits, etc…my computer sits there, gazing at those elements, and the data is more or less worthless…
Which brings me to:
Retina Burn
Eye tracking is not a new technology. One of my roommates works at a lab where they do experiments with eye tracking tools. From what he tells me, the eye tracking gear in the lab is pretty expensive and heavy-weight.
But check this out:
and this:
and this:
So it looks like a webcam and some clever software can do the eye tracking trick too. This stuff is probably far less accurate than what my roommate uses – but it’s lighter, and cheaper, and therefore more likely to find its way into homes. So it looks like the day is coming where I’ll eventually be able to use my eyes to control my mouse cursor.
But, more interestingly, this more or less solves the problem with my Screen Age idea: this technology can tell when you’re looking at the screen. And it can give a pretty good guess about what area of the screen you’re looking at.
I wonder if collecting this information from code reviewers would be useful – what exact parts of the code have they looked at? And for how long? What have they missed? What did they gloss over?
UPDATE: Karen Reid has also brought to my attention the possibility of using this technology to see how TAs grade assignments by tracking what they’re looking at. Hm…
A few posts ago, I mentioned what I think of as the Achilles’ Heel of light-weight code review: the lack of feedback over dependencies that can/will be impacted by a posted change. I believe this lack of feedback can potentially give software developers the false impression that proposed code is sound, and thus allow bugs to slip through the review process. This has happened more than once with the MarkUs project, where we are using ReviewBoard.
Wouldn’t it be nice…
Imagine that you’re working on a “Library” Rails project. Imagine that you’re about to make an update to the Book model within the MVC framework: you’ve added a more efficient static class method to the Book model that allows you to check out large quantities of Books from the Library all at once, rather than one at a time. Cool. You update the BookController to use the new method, run your regression tests (which are admittedly incomplete, and pass with flying colours), and post your code for review.
Your code review tool takes the change you’re suggesting, and notices a trend: in the past, when the “checkout” methods in the Book model have been updated, the BookController is usually changed, and a particular area in en.yml locale file is usually updated too. The code review tool notices that in this latest change, nothing has been changed in en.yml.
The code review tool raises its eyebrow. “I wonder if they forgot something…”, it ponders.
Now imagine that someone logs in to review the code. Along with the proposed changes, the code review tool suggests that the reviewer also take a peek at en.yml just in case the submitter has missed something. The reviewer notices that, yes, a translation string for an error message in en.yml no longer makes sense with the new method. The reviewer writes a comment about it, and submits the review.
The reviewee looks at the review and goes, “Of course! How could I forget that?”, and updates the en.yml before updating the diff under review.
Hm. It’s like a recommendation engine for code reviews…”by the way, have you checked…?”
I wonder if this would be useful…
Mining Repositories for Predicting Impact Analysis
This area of research is really new to me, so bear with me as I stumble through it.
It seems like it should be possible to predict what methods/files have dependencies on other files based on static analysis, as well as VCS repository mining. I believe thishasbeentried in various forms.
I wonder if such a tool would be accurate… and, again, would it be useful? Could it help catch more of the bugs that the standard light-weight code review process misses?
Remember thoseresearchquestionsGreg had me and Zuzel come up with? He’s asked us to select one, and find some tools that could help us in an experiment to answer that question.
Originally, my favourite question was my second one: in the courses where students work in teams, why aren’t the instructors providing or encouraging code review tools?
I’ve already received twoanswers to this question from instructors here at UofT.
Greg has warned me that this line of inquiry might be a bit too shallow. He warns that it’s possible that, when asked about code review, they’ll shrug their shoulders and say that they just don’t have time to teach it on top of everything else. Karen Reid’s response echos this warning, somewhat.
And maybe Steve Easterbrook has a point – that code review is hard to assess as an assignment. It seems he’s at least tried it. However, it appears that he was using fragments of Fagan Inspection reports as his measuring stick rather than the reviews themselves. I assert that this is where light-weight code review tools could be of some service: to actually see the conversation going on about the code. To see the review. I also assert that such a conversation is hard to fake, or at least, to fake well.
So, just go with me on this for a second: let’s pretend that Steve is going to teach his course again. Except this time, instead of collecting fragments of Fagan Inspection reports, he uses something like ReviewBoard, Crucible, or Code Collaborator to collect reviews and conversations. Would it be worth his time? Would it benefit the students? Would they see the value of code review?
Reading this blog post, it seems that the Basie folk first got on the ReviewBoard band wagon because Blake Winton set a good example as a code reviewer. I remember talking to Basie developer Bill Konrad this summer, and him telling me that he’d never go back after seeing the improvement in code quality.
Because that’s the clincher – getting the students to see the value. You can’t make a horse drink, and you can’t get students to use a tool unless they find it useful. And how do you show that to them? How do you show them the value without having to call in Blake Winton? How do you make them see? And how do you make the process painless enough so that instructors don’t have to worry about teaching a new, confusing tool?
One of the comments on Greg’s post says the following:
My feeling is that the easier it is to review code, the more interest you’ll see from students.
Maybe that’s really all you need.
So, how about this for an experiment – take a class of students who are working on group assignments, and set them up a copy of a light-weight code review tool. Get one of the first assignments in the class to involve a review so that they need to use the software at least once. Now track the usage for the rest of the semester…do they still use it? Do some use it, and not others? If so, do the ones who use it tend to receive higher grades? Or is there no difference? What is the quality of their reviews when the reviews are not being marked? What will the students think of review tool once the course is completed?
I think it’s simple enough, and I can’t believe I didn’t think about it earlier.
Quite a few choices for the review tool. And I wasn’t even digging that deeply. Perhaps I should do a quick survey across all of them to see which one would be the best fit for a CS course. Perhaps.
So I had my weekly meeting with my supervisor, and fellow students Zuzel and Jon Pipitone. Something interesting popped up, and I thought I’d share it.
If it wasn’t already clear, I dig code review. I think it really helps get a team of developers more in tune with what’s going on in their repository, and is an easy way to weed out mistakes and bad design in small chunks of code.
But there’s a fly in the soup.
This semester, the MarkUs project has been using ReviewBoard to review all commits to the repository. We’ve caught quite a few things, and we’ve been learning a lot.
Now for that fly:
A developer recently found a typo in our code base. An I18n variable was misspelled in one of our Views as I18n.t(:no_students_wihtou_a_group_message). One of our developers saw this, fixed the typo, and submitted the diff for review.
I was one of the reviewers. And I gave it the green light. It made sense – I18n.t(:no_students_wihtou_a_group_message) is clearly wrong, and I18n.t(:no_students_without_a_group_message) is clearly what was meant here.
So the review got the “Ship It”, and the code was committed. What we didn’t catch, however, was that the locale string was actually named “no_student_without_a_group_message” in the translation file, not “no_students_without_a_group_message”. So the fix didn’t work.
This is important: the diff looked good, but the bug remained because we didn’t have more information on the context of the bug. We had no information about I18n.t(:no_students_without_a_group_message) besides the fact that I18n.t(:no_students_wihtou_a_group_message) looked wrong.
Which brings me back to the conversation we had yesterday: while it seems plausible that code review helps catch defects in small code blocks, does the global defect count on the application actually decrease? Since ReviewBoard doesn’t have any static analysis tools to check what our diffs are doing, isn’t it plausible that while our diffs look good, we’re not preventing ourselves from adding new bugs into the code base?
So, the question is: does light-weight code review actually decrease the defect count across an application as a whole?
If not, can we augment these code review tools so that they’re more sensitive to the context of the diffs that are being reviewed?
When we started using ReviewBoard with MarkUs a few months back, all of a sudden, commits to the repository seemed to slow down: we would take more time cleaning up our code, and polishing it for others to see.
Our commits were usually quite large too. This is because we were all working on different sections of the code, and we wanted to commit stuff that “instantly worked” and was “instantly perfect”. So after days of silence, 1000 lines of code would suddenly go up for review…and as Jason Cohen can probably tell you, the number of defects found during review decreases as the amount of code to look at increases. So, the reviewer would skip through 1000 lines, assume most of it was OK, and give it the Ship It.
Yeah, I know. Awful. I wonder if this is a standard newbie mistake for student groups just starting out with code review…
So, study idea:
Have two separate groups working on some assignment. Have Group 1 commit to their repository without any review process. Have Group 2 do pre-commit reviews using a tool like ReviewBoard.
Now check out the size, frequency, and readability of the repository diffs of each group. Might generate some interesting data.
Anyhow, in our defence, we seem to have calmed down on MarkUs. Diffs up for review are pretty small, and get posted relatively frequently. Using ReviewBoard on MarkUs has made me a believer. Testify!
How about I just round up all of the instructors who teach courses with group assignments, and ask them why code review tools aren’t provided or encouraged. Or maybe they’ve tried, but they ran into a stumbling block. Or perhaps the whole idea of using code review tools flies in the face of some important teaching method.
I won’t know until I ask. So why not just ask?
It might not be a quick, sharp, clever scientific study, but it sure might generate some interesting material for examination.
Our last hostel, back in Wroclaw29-Jun-2009 08:01, FUJIFILM FinePix A345, 2.81, 5.8mm, 0.5 sec, ISO 118
NOTICE
Persons attempting to find a motive in this narrative will be prosecuted; persons attempting to find a moral in it will be banished; persons attempting to find a plot in it will be shot.
By order of the author,
Per G. G., Chief of Ordinance
