Arnt Gulbrandsen
About meAbout this blog

Reviewing code and people

Some years ago an Indian friend of mine was invited to speak at a conference here. He applied for a visa, then the visa application was rejected. He got someone to phone around behind the scenes, and learned the real reason for the rejection.

The decision made sense.

My friend was an expert in his field and could easily get a job here. The fact that he was invited to speak at that conference proved his expertise, and his connections here proved that he could easily get a job. Further, my friend was single (on paper), did not have a highly paid job in India, and he was young and easily able to move. The visa application reviewer quite sensibly inferred that that my friend had the option of not returning.

The visa application reviewer used this decision matrix when processing the application:

Application acceptedApplication rejected
Friend planned to returnNeutralNone
Friend planned to overstayNegativeNone

This matrix leaves out the impact on the conference and on my friend, because their interests did not matter when the decision was taken. The government at the time really wanted to reduce the number of people who overstayed their visas, and almost ignored other factors when it made the decision matrix. It didn't particularly care to please my friend, or to displease him, it wanted to reduce the number of overstayers.

It's easy to curse the xenophobic so-and-sos who guard borders, but when I look at that matrix, I can understand why the reviewer decided to reject the application, if the chance of overstaying seemed even just a little above zero.

Code reviews are like that in some companies. They can be easy to curse, too.

Patch acceptedPatch rejected
Patch worksNeutralNone
Patch is brokenNegativeNone

Given that matrix, of course each and every meeting is more important than getting progress on the patch. Of course any minor objection is enough to block the patch or ask for another revision. Some people say things like if you haven't found any problems, you haven't reviewed properly. I think that when someone can say that with a straight face, the team is secretly using the decision matrix above.

For the next visa application my friend changed the matrix. Among other things, he arranged for a Sir Humphrey Appleby to telephone the consulate and explain that the minister hopes you will treat this young man favourably.

Application acceptedApplication rejected
Friend planned to returnNeutralMinister displeased
Friend planned to overstayNegativeMinister displeased

My friend got his visa. It's always good to have friends in the foreign ministry. It's good to have an appropriate decision matrix, too.

I have no simple hack to balance the matrix for a code review. But balance is necessary, that's one of the keys to useful code reviews. If the matrix is skewed one way, the code will stall, if the other way, it will acquire too much technical debt.

Rewriting history

The commits I ranted about yesterday aren't very large compared to what I wrote. More than 90% of the code I wrote disappeared while I merged commits for review.

The biggest block of code that disappeared was a false start. I had an idea for how to solve a problem, wrote much code, and eventually saw that the program had become lopsided. The percentage of code dedicated to that one problem was far too large. Once I understood that I quickly found a much better approach. Smaller. More robust.

That detour will not be visible in the version history my colleagues can see. That history shows a different Arnt, one that went more or less straight to the right design. […More…]

Code review, Gerrit's way

To the New Yorker, this is a neat way to start a story: A few years ago, at a Las Vegas convention for magicians, Penn Jillette, of the act Penn and Teller, was introduced to a soft-spoken young man named Apollo Robbins, who has a reputation as a pickpocket of almost supernatural ability. Jillette, who ranks pickpockets, he says, a few notches below hypnotists on the show-biz totem pole, was holding court at a table of colleagues, and he asked Robbins for a demonstration, ready to be unimpressed. […More…]

Herding cats

The daily standup and periodic sprint planning in Scrum expose ideas to more people. There's more chat about things. Methodic code review pushes in the same direction.

That has many good effects. One which may not be so good is that dubious features are easily killed before they're implemented, or rejected when the first part is reviewed. Sometimes that makes me unhappy.

Dubious features are Schrödinger's cats: They can be anything from damaging to insanely great.

Insanely great features are insanely great in hindsight, but the ones I've written weren't great in advance, and I fear most of them wouldn't have passed the Scrum process. It's so easy to say no and concentrate on the sprint story. An answer like write it and see, we can always ditch it afterwards isn't in the spirit of scrum.

I suppose that's no bad thing, overall. More deadlines met. But fewer inspired features.