Mike Conley's Blog

According to the timeline, I’m still in the community-bonding period for GSoC.  Coding for my project is supposed to start sometime towards the end of May.

So I’m using the time to do the following:

• Close as many small, easy tickets as I can for the  upcoming Review Board release.  I’ve already posted some patches for review.  More forthcoming.
• Get to know the tools I’ll be using.  Review Board is hosted on Github, and I’m relatively new to the whole DVCS thing.  I’ve been figuring out how to use Git, how to post patches, merging, branching, etc.
• Get to know the area I’ll be working in.  I’ve been figuring out how Django apps organize themselves.  I’ve also drawn up a map of the current state of the extension framework to help me visualize it.
• Get to know the other developers working on Review Board.  I’ve been hanging out in the #reviewboard-soc FreeNode IRC channel.  Very nice, and helpful people to work with.
• Develop a plan of attack for my project

And this last point is the one I want to talk about.

The Review Board Extension Life-Cycle

An extension isn’t just some isolated piece of code that gets crammed into an application.  When you’ve got multiple extensions already installed and running, installing and activating a new extension is like introducing a new animal into an ecosystem.  You have to make sure that your new animal plays nice with the others, and that, in the morning, there won’t be a pile of rotting corpses where your application used to be.

I may have gotten carried away with my metaphor.

So let’s look at what I’m envisioning as the life-cycle for a Review Board extension.  I’ll start right from the top.

Getting the Extension

Ideally, this will work as nicely as WordPress’s implementation:  a Review Board administrator is given a catalog of extensions to choose from within the Administrator interface, and one click later, the desired extensions are downloaded and ready to be installed.

For all you system administrators out there, that last idea might make your toes curl.  A Review Board administrator is not necessarily a system administrator, and the system administrator knows what he/she likes on their machine.  An application that can go and download other applications can be dangerous.  We have to ensure that the application that we’re downloading is the one we’re trying to download.  The last thing we need is some man-in-the-middle to do something cute and bork the code review machine.  And we want to ensure that the extension functions as advertised.  No hidden features.  No self-destruct mechanisms.  No back doors.

Do I have a plan for this part?  Well….no, not really.  I don’t imagine I’ll get that far – I consider it a little out of my scope.  So, for my project, I think it’ll satisfy if the system admin (or Review Board admin) can manually download the extension, decompress it, and place it where Review Board can work with it.  That other stuff can come later (and I’ll try to design so that it can come later easily).

Installing the Extension

Ok, so at this point, we’ve got our extension downloaded in a place where Review Board can see it.

So now what?

Now we need to install the extension.  To me, that means letting the extension put its roots into the RB install by creating database tables, preparing initial data, and generally doing everything to make conditions suitable for the extension to function.

When an extension install begins, I imagine it will consider the following questions (in no particular order):

1. Do I (the extension) depend on other extensions to function?  If so, are those extensions present?  If not, let the user know so that they can go get them.
2. Are there some extensions already installed that will conflict with me, or make me behave badly?  If so, let the user know so that they can either remove that conflict, or find an alternative extension.
3. Is the user entirely aware of what I can do?  Make sure that my capabilities, limitations, and behavioural quirks are known to the user.

Once those 3 questions are answered, and everything is looking good for the install, the extension will create the database tables it needs (if any) in order to function.  If anything goes wrong during this process, the database changes will be rolled back, and the user will get a full read out about what went wrong.

If nothing goes wrong, the extension will be installed.  The user might then be asked to set some initial operating parameters for the extension.

Ok great – the extension is installed.  Now what?

Activating the Extension

For something that sounds so dramatic, the explanation about what happens is pretty short:  Review Board simply becomes aware that the extension is activated, and passes data through the necessary hooks in order for the extension to function properly.  Upon activation, the extension should do a quick double-check to ensure that all prerequisites for the extension have been met.  This is because we can’t trust users to activate an extension immediately after downloading them.

The Extension Runs

While it’s activated, the extension will probably react to various events that happen on Review Board.  Tables will be updated.  View methods will be run.  Templates will be rendered.

If anything, ever, goes horribly wrong with an extension, the following will happen:

1. A log entry will be written, dumping the error, and information about what the user was trying to do
2. An error message will be displayed to the user, trying to tell them what exactly happened
3. The extension (and its dependents) will be deactivated.  They will no longer react to events on Review Board.  Their tables will still be there, the settings will still, but the extension will be, in essence, asleep.  Dormant.  Non-reactive.

The administrator could try to reactivate the extension at this point.  They might try to contact the extension developer for support.

The Extension is Un-installed

If the user wants to rid themselves of an extension, they must first deactivate it.  This will put it (and its dependents) in the dormant state.  It just switches them off, nothing else.

Deactivated extensions can then be un-installed.  If the user chooses to un-install the extension, the extension database tables and settings will be wiped out.

The extension itself won’t be deleted though – at least, not within the scope of my project.  The extension files will need to be removed from Review Board manually.

At this point, any dependents that this uninstalled extension had will no longer be able to be activated.

Anyhow, that’s how I envision the life-cycle.  It’s my first go at it, so I’d love to hear some feedback if you have any.

Be the first to like.

Like

Unlike

If you didn’t already know, Review Board is an open-source web-based code review tool.  The MarkUs Team has been using Review Board for pre-commit code review for about a year now.  This has given the team a number of advantages:

1. For a team that usually has a 4 month turnover, this allows us to quickly get new team members up to speed with how to contribute to MarkUs.  We review every change that they propose, and give them tips/guidance on how to make it fit in well with the application.  They learn, and the applications code stays healthy.
2. We catch defects before they enter the code base.  Simple as that.
3. We get a good sense of what other people are working on, and what is going on in the code.  Review Board has become a central conversation and learning hub for the developers on the MarkUs team.

So, the long and the short of it:  I like Review Board.  Review Board helps us write better code.  I want to make Review Board better.

So what am I proposing?

How to Avoid A Bloated Software Monster

You can never make some people happy.

No matter how decent your software is, someone will eventually come up to you and say:

Wow!  Your software would be perfect if only it had feature XYZ!  Sadly, because you don’t have feature XYZ, I can’t use it.  Please implement, k thx!

And so you either have to politely say “no”, and lose that user, or say “yes”, and add feature XYZ to the application.  And for users out there who don’t need, or don’t care about feature XYZ, that new feature just becomes a distraction and adds no value.  Make this happen a bunch of times, and you’ve got yourself a bloated mutha for a piece of software.

And we don’t want a bloated piece of software.  But we do want to make our users happy, and provide feature XYZ for them if they want it.

So what’s the solution?  We provide an extension framework (which is also sometimes called a plug-in architecture).

An extension framework allows developers to easily expand a piece of software to do new things.  So, if a user wants feature XYZ, we (or someone else) just creates and make available an extension that implements the feature.  The user installs the extension, activates it, and bam – our user is happy as a clam with their new feature.

And if we make it super-easy to develop them, third-party developers can write new, wonderful, interesting extensions to do things that…well, we wouldn’t have considered in the first place. It’s a new place for innovation.  What’s that old cliché?

If you build it [the plug-in framework], they will come [the third-party developers who write awesome things]

And the developers do come.  Just look at Firefox add-ons or WordPress plugins.  Entire ecosystems of extensions, doing things that the original developers would probably have never dreamed of doing on their own.  Hell, I’ve even written a Firefox add-on. And users love customizing their Firefox / WordPress with those extensions.  It adds value.

So we get wins all over the place:

• Our user gets their feature
• The software gets more attractive because it’s flexible and customizable
• The original software developers get to focus on the core piece of software, and let the third-party developers focus on the fringe features

And this is where I think I can help Review Board.

(Before I go on, if you’re interested, here’s another article on the how and the why of plug-in architectures)

Review Board Extensions

So if you look at the Review Board Wiki, or glance at the mailing lists you see numerous requests from users for new features, for example:

It would be nice if the review board had a “next comment” button that is always available to click, or had a collapse/expand button. This would make it easier to see other people’s comments in cases like this.

…

It will be nice to have post-commit support. Instead of every post-commit review being a separate URL, if we could setup default rules for post-commit reviews to update an existing review providing the diff-between-diff features, it would be very useful.

The Review Board developers could smell the threat of bloated feature-creep from a mile away.  So, in a separate branch, they began working on integrating an extension framework into Review Board.

The extension branch, however, has been gathering dust, while the developers focus on more critical patches and releases.

My GSoC proposal is to finish off a draft of the extension framework, document it, and build a very simple extension for it.  My simple extension will allow me to record basic statistics about Review Board reviewers – for example, how long they spend on a particular review, their inspection rate, etc.

Having been a project lead MarkUs for so long, it’s going to be a good experience to be back on “the bottom” – to be the new developer who doesn’t entirely have a sense of the application code yet.  It’s going to be good to go code spelunking again.  I’ve done some preliminary explorations, and it’s reminding me of my first experiences with MarkUs.  Like a submarine using its sonar, I’m slowly getting a sense of the code terrain.

I’ll let you know what my first few sweeps find.

Be the first to like.

Like

Unlike

One of my potential research experiments involves augmenting ReviewBoard, and so, I’ve been studying the ReviewBoard code.

I want to give ReviewBoard the ability to record certain statistics – for example, the number of defects in a review request, number of defects found per reviewer, defect density, inspection rate, etc…

It turns out I’m not the only one who wants this to happen.  Check out this thread.

So it turns out that the devs building ReviewBoard are planning on integrating an extension framework.  Theoretically, users could then write their own extensions to customize ReviewBoard as they see fit.  Think WordPress plugins, but for a code review tool.  Neat.

According to the above link:

A lot of this [extension framework] already exists in a private development branch, and it will be one of our primary focuses as soon as 1.0 goes out.

However, I found the following quote from a relatively recent mailing list message:

[with regards to the extensions framework]…We have to finish building it, testing
it, getting other features we have planned into the release, and performing
the release. So, we’re looking at a year or more. But I don’t see how we’d
get the functionality you want short-term.

Hrmph.  My thinking:  maybe I can help them with it, and we can crank this puppy out sooner.

So where is this private branch?  It actually took a little bit of detective work…

Detective Work

I started by looking at the ReviewBoard repo on Github, which I assumed that the extensions branch would fork from.

I was right – check out the fork network.  There’s a branch helpfully labeled “extensions” forked by davidt, one of the ReviewBoard devs.

Two observations:

1. This fork hasn’t been updated in about a month
2. The extensions stuff references stuff from Djblets which doesn’t exist in the Djblets master branch

So I perform the same exercise – I glance at the Djblets fork network, and see the handily named “extensions” fork that is in davidt’s git repo.  Nice.

Ok, so those are the forks/branches that I’m going to grab.

A Fluke

So, funny story:  I was able to get this “extension” version of ReviewBoard working on my work machine.  But it was by a complete fluke – this extension version of Djblets is actually missing some pieces.  The only reason it worked for me was because I accidentally ran with the master checkout of Djblets first.  This errored out, and also generated a bunch of compiled Python.  I then switched to the extensions branch, which left the compiled Python behind.  The compiled Python filled all of the missing pieces that are in the extensions branch of Djblets, and so it ran.

For a guy who is pretty new to both Git and Django, I found that problem pretty frustrating to track down.  Especially when I started writing up the checkout instructions… big thanks to Zuzel for her invaluable (and patient) Django guidance.

Anyhow, if you’re interested in the steps to failure, here they are…

Setting Up (for failure)

I’m running Ubuntu Karmic.  I have Git installed.

1. First of all, this document is invaluable
2. Check out django_evolution from SVN to some local directory
3. Put path to django-evolution into PYTHONPATH (export PYTHONPATH=$PYTHONPATH:/…/django-evolution-read-only/)
4. Get Python Setuptools (sudo apt-get install python-setuptools)
5. With easy_install, get nose, paramiko, recaptcha-client, Sphinx (sudo easy_install nose paramiko recaptcha-client sphinx)
6. Clone original master Djblets repo (git clone git://github.com/djblets/djblets.git) and CD into djblets directory
7. Add davidt’s remote Djblets branch (git remote add davidt git://github.com/davidt/djblets.git)
8. Fetch davidt’s repo  (git fetch davidt)
9. Track davidt’s extension branch for djblets (git branch extensions davidt/extensions)
10. In djblets root dir, type:  sudo python setup.py develop
11. This should install Django too.  The ReviewBoard install doc says to install from Subversion, but this will do for now.
12. Clone ReviewBoard git repo (git clone git://github.com/reviewboard/reviewboard.git) and Cd into it
13. Add davidt’s remote Reviewboard branch (git remote add davidt git://github.com/davidt/reviewboard.git)
14. Fetch davidt’s repo  (git fetch davidt)
15. Track davidt’s extensions branch of reviewboard (git branch extensions davidt/extensions)
16. Switch to extensions branch (git checkout extensions)
17. So reviewboard/reviewboard/extensions should have stuff in it.
18. python ./contrib/internal/prepare-dev.py
19. Manually create the directory reviewboard/reviewboard/htdocs/media/ext.  ReviewBoard expects it and will complain if it isn’t there.
20. Run ./contrib/internal/devserver.sh
21. Go to localhost:8080

If you’re like me, you’ll see an error about “no module named urls”.  This is because the extension branch of Djblets is missing urls.py in its djblets/log directory.  As you can see, urls.py exists in the master Djblets branch.

So I’ve accidentally got it running.  But this version of ReviewBoard I’ve created is like Frankenstein’s monster – it’s a sad, stitched together monstrosity that should probably never see the light of day.

I tried to merge the master branch of Djblets and ReviewBoard into the extensions branch, and then realized that I had no idea what I was doing.  The last guy you want doing a merge is a guy who doesn’t know what he’s doing.

So that’s where I stand with it.  Kind of a sad affair, really.  I’ve posted on the ReviewBoard developer mailing list asking for some guidance, but all is quiet so far.

In the meantime, I’ll continue studying the code when I can.  The extensions stuff is actually pretty far along, in my opinion – but what do I know, I’ve never built one.  I’ll also take a peek at some other examples of extension frameworks to see what I can learn about implementation (that’s right – I’m talking about you, WordPress).

I’ll let you know what I find.

1 person likes this post.

Like

Unlike

A little while back, I wrote about an idea that a few of us had been bouncing around:  peer code review achievements.

It started out as a bit of Twitter fun – but now it has evolved, and actually become a contender for my Masters research.

So I’ve been reading up on reputation and achievement systems, and it’s been keeping me up at night.  I’ve been tossing and turning, trying to figure out a way of applying these concepts to something like ReviewBoard.  Is there a model that will encourage users to post review requests early and often?  Is there a model that will encourage more thorough reviews from other developers?

An idea eventually sprung to mind…

Idea 1:  2 Week Games

In a speech he gave a few years ago, Danc of The Last Garden placed the following bet:

• If an activity can be learned…
• If the player’s performance can be measured…
• If the player can be rewarded or punished in a timely fashion…
• Then any activity that meets these criteria can be turned into a game.

Let’s work off of this premise.

Modeled on the idea of a sprint or iteration, let’s say that ReviewBoard has “games” that last 2 weeks.

In a game, users score points in the following way:

• Posting a review request that eventually gets committed gives the author 1 point
• A review request that is given a ship-it, without a single defect found, gives the author The 1.5 Multiplier on their total points.  The 1.5 Multiplier can be stolen by another player if they post a review request that also gets a ship-it without any defects being found.
• Any user can find/file defects on a review request
• A defect must be “confirmed” by the author, or “withdrawn” by the defect-finder.
• After a diff has been updated, “confirmed” defects can be “fixed”.  Each fixed defect gives the defect-finder and author 1 point each.

After two weeks, a final tally is made, achievements / badges are doled out, and the scores are reset.  A new game begins.  Users can view their point history and track their performance over time.

Granted, this game is open to cheating.  But so is Monopoly.  I can reach into the Monopoly bank and grab $500 without anybody noticing.  It’s up to me not to do that, because it invalidates the game.  In this case, cheating would only result in bad morale and a poorer piece of software.  And since scores are reset every two weeks, what’s the real incentive to cheat?

Idea 2:  Track My Performance

I’ve never built a reputation system before – but Randy Farmer and Bryce Glass have.  They’ve even written a book about it.

Just browsing through their site, I’m finding quotes that suggests that there are some potential problems with my two week game idea.  In particular, I have not considered the potentially harmful effects of displaying “points” publicly on a leader-board.

According to Farmer / Glass:

It’s still too early to speak in absolutes about the design of social-media sites, but one fact is becoming abundantly clear: ranking the members of your community-and pitting them one-against-the-other in a competitive fashion-is typically a bad idea. Like the fabled djinni of yore, leaderboards on your site promise riches (comparisons! incentives! user engagement!!) but often lead to undesired consequences.

They go into more detail here.

Ok, so let’s say that they’re right.  Then how about instead of pitting the reviewers against one another, I have the reviewers compete against themselves?

Ever played Wii Sports?  It tracks player performance on various games and displays it on a chart.  It’s really easy to see / track progress over time.  It’s also an incentive to keep performance up – because nobody wants to go below the “Pro” line.

So how about we just show users a report of their performance over fixed time intervals…with fancy jQuery charts, etc?

So what?

Are either of these ideas useful?  Would they increase the number of defects found per review request?  Would they increase the frequency and speed of reviews?  Would they improve user perception of peer code review?  Would it be ignored?  Or could it harm a team of developers?  What are the benefits and drawbacks?

If anything, it’d give ReviewBoard some ability to record metrics, which is handy when you want to show the big boss how much money you’re saving with code review.

Might be worth looking into.  Thoughts?

Be the first to like.

Like

Unlike

If you recall, I’m looking at author preparation in code review, and whether or not it impairs the ability of reviewers to perform objective reviews effectively.

If this is really going to be my research project, I’ll need to get my feet a bit more wet before I design my experiment.  It’s all well and good to say that I’m studying author preparation…but I need to actually get a handle on what authors tend to say when they prepare their review requests.

So how am I going to find out the kinds of things that authors write during author preparation?  The MarkUs Project and the Basie Project both use ReviewBoard, so it’ll be no problem to grab some review requests from there.  But that’s a lot of digging if I do it by hand.

So I won’t do it by hand.  I’ll write a script.

You see, I’ve become pretty good at manipulating the ReviewBoard API.  So mining the MarkUs and Basie ReviewBoard’s should be a cinch.

But I’d like to go a little further. I want more data.  I want data from some projects outside of UofT.

Luckily, ReviewBoard has been kind enough to list several open source projects that are also using their software.  And some of those projects have their ReviewBoard instances open to the public.  So I just programmed my little script to visit those ReviewBoard instances, and return all of the review requests where the author of the request was the first person to make a review.  Easy.

Besides MarkUs and Basie, I chose to visit the Asterisk,  KDE, and MusicBrainz projects.

Asterisk was a crapshoot – of all of their review requests, not a single one returned a positive.

But I got a few blips on the others. Not many, but a few.

I read all of the author preparation for each blip, and broke down what I read into some generalizations.

So, now to the meat:  here are some generalizations of what the authors tended to say, in no particular order.  I’ve also included a few examples so you can check them out for yourselves.

“Here’s why I did this”

The author makes it explicit why a change was made in a particular way.

Examples:

• Basie:  425
• KDE: 2447
• MarkUs: 197
• MusicBrainz: 130

“Here’s what this part does…”

The author goes into detail about what a portion of their diff actually does.

Examples:

• Basie: 418
• KDE: 1515
• MarkUs: 139
• MusicBrainz: 341

“Can I get some advice on…”

The author isn’t entirely sure of something, and wants input from their peers.

Examples:

• Basie:  463
• KDE:  577
• MarkUs: 146
• MusicBrainz: 170

“Whoops, I made a mistake / inserted a bug.  I’ll update the diff.”

The author has found a mistake in their code, and either indicates that they’ll update the diff in the review request, or change the code before it is committed.

• Basie: 436
• KDE: 483
• MusicBrainz: 185

“Whoops – that stuff isn’t supposed to be there.  Ignore.”

The author has accidentally inserted some code into the diff that they shouldn’t have.  They give their assurances that it’ll be removed before committing – reviewers are asked to ignore.

Examples:

• KDE: 674
• MarkUs: 241
• MusicBrainz: 350

“Before you apply this patch, you should probably…”

The author believes that the reviewers will need to do something special, or out of the ordinary, in order to apply the diff.

• Basie: 461
• KDE: 1107
• MusicBrainz: 179

“…hello?”

The review request has been idle for a while without a single review.  The author pings everybody for some attention.

Examples:

• KDE: 619
• MarkUs: 215

Anyhow, those are the general patterns that stand out.  I’ll post more if I find any.

Have you seen any other common patterns in author preparation?  What would you say, if you were preparing your code for someone else to review?  I’d love to hear any input.

PS:  If anyone is interested in getting the full list of author prepared review requests for these 4 projects, let me know, and I’ll toss up all the links.

Be the first to like.

Like

Unlike

In my opinion, at least for The MarkUs Project, post-commit code review would probably be analogous to putting on your shoes before your pants.  And though I mentioned earlier that there is plenty of preference for post-commit, I forgot to include this juicy little tidbit.

Click here to read one of the developers of ReviewBoard state his case for pre-commit code review.

To each their own.  But I dig his points.

Be the first to like.

Like

Unlike

Discussion Plays

I have seen plays that have very clear stories, and very clear plots.  I leave the theatre knowing what has happened, and I can be pretty confident that the people who sat around me in the theatre all got the same message as I did.

I have also seen plays that are completely the opposite.  There doesn’t appear to be a story.  There doesn’t appear to be plot.  There are no real characters.  For these plays, all of a sudden, I have to do the work in order to make sense of it all.  And you can be pretty sure that every single audience member got something different out of it.

I want to talk about this second kind of play.  For now, I’m going to call this kind of play a discussion play, because for me, the best part about these kinds of plays is the discussion I have with my friends afterwards. We’ll all sit down in a restaurant or a cafe, order some food, and try to figure out what the hell we just saw.  Theories are tossed around.  Everybody brings their own unique impressions and observations to the table.  A very rich ecosystem of ideas develops.

Back to Peer Code Reviews

(trust me, this all ties together in the end)

When Jason Cohen did his Peer Review at Cisco Study, he noticed that code that had been prepared by the author for review seemed to have a lower defect density than code that had not been prepared.

What do I mean by prepared?  I’ll let Jason Cohen explain:

The idea of “author preparation” is that authors should annotate their source code before the review begins.  Annotations guide the reviewer through the changes, showing which files to look at first and defending the reason and methods behind each code modification.  The theory is that because the author has to re-think all the changes during the annotation process, the author will himself uncover most of the defects before the review even begins, thus making the review itself more efficient.  Reviewers will uncover problems the author truly would not have thought of otherwise.

(Best Kept Secrets of Peer Code Review, p80-81)

Looking at the data, author preparation does seem to have a palpable effect.  As Cohen notes, “for all reviews with at least one author preparation comment, defects density is never over 30; in fact the most common case is for there to be no defects at all!”.

The study has two explanations for this:

1. Authors gave their code such a thorough look while annotating them, that most defects were eliminated right off the bat.
2. Since authors were actively explaining, or defending their code, this sabotaged the reviewers ability to do their job effectively.

Cohen buys into the first explanation.  He writes:

A survey of the reviews in question show the author is being conscientious, careful, and helpful, and not misleading the reviewer.  Often the reviewer will respond to or ask a question or open a conversation on another line of code, demonstrating that he was not dulled by the author’s annotations.

I have huge respect for this study.  But I don’t entirely buy this explanation.  As Cohen later mentioned in an email to me, this conclusion is not derived from a controlled experiment, and also suffers from selection bias.

Back to those Discussion Plays

One of the worst things that can happen to me before going into a discussion play is for someone who has already seen it to tell me their impressions of what they thought was going on.  As soon as I hear their opinion, my own objectivity is compromised.  Whether I want to or not, I’ll have their impressions in the back of my mind, and I’ll be using it as a measuring stick or reference point for my own opinions and critiques. They’ve carved a cognitive path through the work, and I’m doomed to notice that path, and react to it.

This is horrible.  This limits me.  This more or less hobbles my ability to contribute something unique to the pool of ideas and criticisms in the after-play discussion.  Every impression I have is tainted by someone else’s first impression.

Don’t get me wrong – I love hearing about everyone’s impressions.  But after I have formed my own. This way, I believe we cover more ground.  A group of us watching a discussion play will carve unique cognitive paths through the work without influencing one another.  When we finally open up and present these paths and ideas to one another over food and drink, I believe we cover more ground.

I have no data to back this up.  Only years of theatre-going experience.

A Code Review Anecdote

I recently received an email from a colleague of mine.  She wanted me to go over some of her Javascript to make sure it was up to snuff, since she was relatively new to the language.  I noticed that she had also sent a copy of the email to another developer who has pretty sharp Javascript chops.

When I finally had some free time, I went back to her email to write up the review.  I felt bad – it was late, and the other reviewer hadn’t made a peep on the email thread, and she was hoping to use the code relatively soon.  So I dove in, wrote my review, and sent it off.

A little while later, the other developer sent me his review, saying:

And here was my answer, which I didn’t send to you so as not to influence your reply.  ;)

So the author of the code received two unique reviews, and neither of them had influenced the other.  When I read his review, I noticed that we covered some similar ground, but a lot of unique ground as well.  I suspect this wouldn’t have been the case had he sent his review to me first.

The Hypothesis

I hypothesize that author preparation in code review sabotages reviewers abilities to objectively carve their own unique cognitive paths through the code.  They see things from the author’s point of view, and this dulls their critical eye.  Because of this, I believe fewer defects are detected.

I will take this hypothesis one step further.

I suspect any review, by the author or otherwise, will taint future reviews.  If someone has already reviewed some code, I suspect this review will impact and possibly limit the ability of other reviewers to look at the code objectively.  Like author preparation, I suspect this prevents reviewers from getting their own unique, valuable first impressions of the code.  And I suspect that this causes some defects to go undetected.

Testing This Hypothesis

It’s a simple idea really.  Take a chunk of code, and get some number of developers to review it.  Take this same code, add some author preparation comments, and get more developers to review it.  Do all of the normal balancing, etc.

The question:  do the number of detected defects drop?  If so, this looks like evidence that author preparation sabotages review ability.

Take the experiment one step further.  Take some code, have someone else review it, and then have participants review this code, having seen the first review.  What happens to the number and type of defects that they find?  What happens if they don’t see that initial review?  What yields high defect detection?

Sounds doable.  Sounds interesting.  Sounds like something that would answer a few questions.

Implications and Ideas

So what if one or both of my hypotheses are true?  What does this mean for peer code review?

Well, if author preparation alone sabotages review ability, then the answer is simple:  don’t let the authors prepare the review.  The code goes up, and they stay silent.

But what if both are true?

An idea:  how about I tweak MarkUs’s ReviewBoard so that reviewers cannot see what other reviewers have said until they’ve given one review?  What would happen to the defect detection numbers?  Would reviewers react negatively to this?  Would there be lots of repetition in the comments?  Sounds like something worth looking into.

I’d love to hear some thoughts on this.  Anyone?

Be the first to like.

Like

Unlike

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:

• It goes against “check in early, check in often“
• “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:

1. 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.
2. 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…
3. 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.
4. 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?

I’m curious to find out.

Unfortunately, ReviewBoard doesn’t let me download diffs through its API just yet…if schoolwork lets up for a few days, I’ll look into changing that.

I’d love to hear your thoughts.

Be the first to like.

Like

Unlike

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?

Be the first to like.

Like

Unlike

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!

Be the first to like.

Like

Unlike