I iz in ur code makin bugz: Code ownership and the pain of code review

Like a little lolcat in a bucket of fudge, when someone goes into your code they think they are being so cute. And they are wrong. You have spent a lot of time in there, thinking hard, being professional. Crafting, testing, fixing. You’ve spent a long time thinking about this and now they think they can make it better but it isn’t better, it is just a little bit different. What is wrong with the code how it is? It works, it is – mostly – tested and it makes sense. Change is only wasting more time that would be better spent on something else. Of course, code review is a good thing, When the reviewer picks up mistakes, but the mistakes are pretty minor. OK so it could be slightly better, but what are we doing, creating a system or the Mona Lisa?

On the other hand, when we look at someone else’s code it is so easy to pick out mistakes. They have been concentrating on making it work, now you can come along with a fresh attitude and suggest all sorts of improvements like refactorings and patterns. You can clean up method names and maybe consolidate a few interfaces, it won’t take long as it already works. Oh and I found a bit of cut-and-paste code that we could get rid of. OK it might need a few more tests as well. And it will be so much better afterwards.. nobody’s code is perfect. I wouldn’t mind if someone made those suggestions. If he doesn’t want to do the work himself then I’ll do it…

Doing a code review is difficult, doing it right is even harder. Even getting started on a code review takes a lot of effort, code ownership is a big habit to break. There are few technical barriers but a lot of social ones. Doing a review means that both sides need to break through some level of politeness and actually have a confrontation. It doesn’t always have to be this way, but the first time it almost certainly will be a direct “you versus them”. After all, if the reviewer doesn’t want to change anything then they probably didn’t look hard enough and if they want to change something then the code writer will — and should — object. After all, unless you sleepwalked into writing that code, you should have a reason for doing it the way you did and you should be able to communicate it.

And if doing a code review is hard, receiving one is harder. Afterwards, the code reviewer thinks “Tick! Another job done and more code cleaned up”. But you have just had your weaknesses exposed in public. You have been shown up  not only for making a mistake, but for refusing to having your code changed, and then – the final indignity – having it pointed out that the code is better afterwards. It seems to me that the older a person is, and the longer they have been working on a piece of code, the more problems there are. This is reasonable, you have more to lose in time and standing. Code review cuts right to the heart of a programmer’s place in the world We all like to be good at what we do, and getting some social standing for technical knowledge is very important. Code review challenges that. It should be no surprise that programmers — normally male, sometimes less than well socialised — get into “antler-locking sessions” over a piece of code. That code is like a child, you have invested a lot of time and effort and pride into it; and no one is going to criticise or – God forbid – correct one of my children without me defending them.

One way to combat that level of high level of personal investment in code is to make sure reviews are frequent and low key. Pair programming might work in this respect as a) the code has already been seen by two people so they share the ownership/pride/effort so level of owenership is down by half and b) the pairing is a kind of review in itself, people are used to being constantly challenged, so it becomes less personal. Another way might be to do three way reviews, as long as the three are reasonably independent – and not too childish – the conflict will be diffused by the third person who will disrupt any serious conflict. The extreme of this might be to review by a panel, but that could be very scary for people as it is now a public confrontation. Code review can be better face to face, or it can be worse. Some people can read signals and moderate their presentation of the review to make it more palatable, after all, it is one thing to think “this is rubbish” it is another to look someone in the face and stick it to them. However, if you are the kind of person who won’t back down, you can reduce a reviewers influence until nothing gets changed just because you are so intimidating. If you meet a reviewer who won’t back down, you might come to blows. Not even kidding, I’ve done it myself.

Code review requires people skills, not always a programmer’s top attribute. Once you realise that programming is creative design work that will never be automated, only abstracted the pain of a code review makes much more sense. Imagine if a architect or a fashion designer or an advertising “creative” was out of the office (in the table football room or sauna or whatever) and came back to find it has all been changed… You would expect tantrums, why expect less of programmers, even if they are slightly more emotionally restrained…

So when you are doing a code review, be moderate. Try not to focus on naming or code layout, unless it really is a problem (like Hungarian notation like psz_a over real names or 500 character lines). Deliver it with a little humility, but, stick to your guns. You aren’t doing anyone any favours by being intimidated into silence. You will end up feeling bitter as well as the code writer, and the code will probably be worse too. Expect the code writer to have a good reason, and try to find it. There is no need to go all open source on it and be brutal thinking that everyone likes people to be direct. They don’t. Don’t expect people to be impersonal about their work, they won’t be. And don’t score points. I’ve seen many an academic presentation disrupted by 70 year old professors attacking some 21 year old grad student over a trivial point just to make them realise they are at the bottom of the social scale. These growing nerds then emulate the alpha-males in their attitudes of passive-aggressive attacks in public: “Well, if you are happy with it like that, then good for you. Some people are comfortable with imprecision. I’ll find someone else who really knows the answer.” Of course, if you review someone who is much better than you, they won’t learn anything, but you just might.

And if you are receiving a review, try to keep an open mind. If they are trying to score points, rise above it; if they suggest a change that you disagree with, think about why they are suggesting it and why you are disagreeing with it. You might find you are the problem. And when it is done, put it behind you. Talk about something else with the reviewer and move on. Trying to get them back when you are the reviewer is a mistake. Nobody will win. If the reviews always degenerate into a slanging match, then you need to try something – or someone – different, but don’t give up reviews.

Of course, the only way to really succeed at a code review is to have trust in the other person. And that won’t come until you actually try it and find out that it isn’t so bad. First we try, then we trust.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s