Canonical Voices

Posts tagged with 'code review'

Jussi Pakkanen

Code review is generally acknowledged to be one of the major tools in modern software development. The reasons for this are simple, it spreads knowledge of the code around the team, obvious bugs and design flaws are spotted early, which makes everyone happy and so on.

But yet our code bases are full of horrible flaws, glaring security holes, unoptimizable algorithms and everything that drives a grown man to sob uncontrollably. These are the sorts of things code review was designed to stop and prevent, so why are they there. Let’s examine this with a thought experiment.

Suppose you are working on a team. Your team member Joe has been given the task of implementing new functionality. For simplicity’s sake let us assume that the functionality is adding together two integers. Then off Joe goes and returns after a few days (possibly weeks) with something.

And I really mean Something.

Instead of coding the addition function, he has created an entire new framework for arbitrary arithmetic operations. The reasoning for this is that it is “more general” because it can represent any mathematical operation (only addition is implemented, though, and trying to use any other operation fails silently with corrupt data). The core is implemented in a multithreaded async callback spaghetti hell that only has a data race on 93% of the time (the remaining 7% covers the one existing test case).

There is only one possible code review for this kind of an achievement.

Review result: Rejected
Comments: If there was a programmer's equivalent to chemical
castration, it would already have been administered to you.

In the ideal world that would be it. The real world has certain impurities as far as this thing goes. The first thing to note is that you have to keep working with whoever wrote the code for an unforeseeable amount of time. Aggravating your coworkers consistenly is not a very nice thing to do and gives you the unfortunate label of “assh*ole”. Things get even worse if Joe is in any way your boss, because critizising his code may get you on the fast track to the basement or possibly the unemployment line. The plain fact, however, is that this piece of code must never be merged. It can’t be fixed by review comments. All of it must be thrown away and replaced with something sane.

At this point office politics enter the fray. Most corporations have deadlines to meet and products to ship. Should you try to block the entry of this code (which implements a Feature, no less, or at least a fraction of one) makes you the bad guy. The code that Joe has written is an expense and if there is one thing organisations do not want to hear it is the fact that they have just wasted a ton of effort. The Code is There and it Must Be Used this Instant! Expect to hear comments of the following kind:

  • Why are you being so negative?
  • The Product Vision requires addition of two numbers. Why are you working against the Vision?
  • Do you want to be the guy that single-handedly destroyed the entire product?
  • This piece of code adds a functionality we did not have before. It is imperative that we get it in now (the product is expected to ship in one year from now)!
  • There is no time to rewrite Joe’s work so we must merge this (even though reimplementing just the functionality would take less effort than even just fixing the obvious bugs)

This onslaught continues until eventually you give in, do the “team decision”, accept the merge and drink yourself unconscious fully aware of the fact that you have to fix all these bugs once someone starts using them (you are not allowed to rewrite it, you must fix the existing code, for that is Law). For some reason or another Joe seems to have magically been transferred somewhere else to work his magic.

For this simple reason code review does not work very well in most offices. If you only ever get comments about how to format your braces, this may be affecting you. In contrast code reviews work quite well in other circumstances. The first one of them is the Linux kernel.

The code that gets into the kernel is being watched over by lots of people. More importantly it is being watched over by people who don’t work for you, your company or their subsidiaries. Linus Torvalds does not care one iota about your company’s quarterly sales goals, launch dates or corporate goals. The only thing he cares about is whether your code is any good. If it is not, it won’t get merged and there is nothing you can do about it. There is no middle manager you can appeal to or HR you can usurp on someone. Unless you have proven yourself, the code reviewers will treat you like an enemy. Anything you do will be scrutinised, analysed, dissected and even outright rejected. This intercorporate fire wall is good because it ensures that terrible code is not merged (sometimes poor code falls through the cracks, though, but such is life). On the other hand this sort of thing causes massive flame wars every now and then.

This does not work in corporate environments, though, for the reasons listed. One way to make it work is to have a master code reviewer who does not care about what other people might think. Someone who can summarily reject awful code without a lengthy “let’s see if we can make it better” discussion. Someone who, when the sales people come to him demanding something to be done half-assed, can tell them to buzz of. Someone who does not care about hurting people’s feelings.

In other words, a psychopath.

Like most things in life, having a psychopath in charge of your code has some downsides. Most of them flow from the fact that psychopaths are usually not very nice to work with.Also, one of the things that is worse than not having code review is having a psychopath master code reviewer that is incompetent or otherwise deluded. Unfortunately most psychopaths are of the latter kind.

So there you have it: the path to high quality code is paved with psychopaths and sworn enemies.

Read more
Daniel Holbach

I talked many times about getting involved with developing Ubuntu and how it can seem daunting and that there’s much to learn. When I talked to contributors who had reached the critical point where they understood what they can do, who they can talk to and how the processes roughly work, most of them said that three things helped them to get to the point:

Code review

Today I want to talk about code reviews. It’s probably the most straight-forward way to learn by osmosis: you easily pick up conventions, distinctions which are made and which processes to follow.

Everybody has to go through code reviews, no matter which team they are in, which company they work for or when they joined the project. Up until a point they get their developer application approved and get upload rights.

This is the reason why code reviews in Ubuntu are so important and why we should constantly strive for timely replies and decisions on review requests.

Sponsoring Queue

The sponsoring queue is reviewed by developers with upload rights. Sometimes it’s very easy to approve a request and upload the package, sometimes it takes a bit longer, especially when you have a comment ping-pong between the reviewer and the patch author.

We came up with a number of points in our documentation which should help keeping the queue manageable:

For Bugs fixing small details, you could do the following:

  1. Ask the contributor to forward the patch upstream.
  2. Open an empty upstream bug task.
  3. Mark the Ubuntu task as ‘Fix Committed’.
  4. Unsubscribe ubuntu-sponsors, or mark the merge proposal status as “Work in Progress”. (Be sure to tell the contributor to reverse the process.)

This will get the review item off the list for the time being and once we can import the code from upstream, it will get fully closed.

We also get requests which are not suitable for the current release period. In this case you could:

  1. Let the contributor know that the patch is not suitable for the current release period.
  2. Unsubscribe ubuntu-sponsors, or mark the merge proposal status as “Work in Progress”. (Be sure to tell the contributor to reverse the process.)
  3. Subscribe yourself to the bug report.
  4. Milestone the bug to ‘later’.
  5. Visit https://bugs.launchpad.net/people/+me/+bugs/?field.milestone%3Alist=196 once the new release opens and upload the fix.

This are just some points which help to keep things on the queue relevant.

Patch Pilots

From the Bazaar team we borrowed the scheme of “patch pilots”. Here’s how they explain how it works: “The word pilot is in the sense of a maritime pilot: we help patches come through congested waters safely in to harbour. The main thing to watch is the bzr active reviews page in Launchpad. When you’re piloting, put some concentrated effort into helping people have a good and satisfying experience contributing to Bazaar. Just how you do that is up to you.

Instead of trying to review each and every bit in the queue – sometimes there are packages you know less about and where you can’t make a decision for example – you try to help nudge the patch along. You help to talk to upstream about it, try to find somebody who can make a decision, etc.

Canonical engineers with upload rights who work on Ubuntu are expected to spend an hour per week on the Ubuntu sponsoring queue, so everybody’s on the hook for having a piloting shift 4h every four weeks. This usually works much better, as you have an extended period of time where you do nothing else. Current patch pilots can be seen in the #ubuntu-devel channel topic.

Up until now I mostly noticed Canonical engineers who did piloted. If you have upload rights and are interested, let me know and I can add you to a preliminary schedule, so you get a reminder mail and you can try it out and see if you like it.

Please all help making this work even better. :-)

Read more
Daniel Holbach

Congratulations everyone, we got a fantastic LTS release out, some of the reactions of our users you can see here. Well done! :)

At the same time understandable, but also worrying is the look at our sponsoring page:

Silently with all the release freezes in place, the number of open sponsorship items has crept up to ~70 again.

If you are an Ubuntu developer or can help out with reviewing patches, please head to the sponsoring queue and help out. Many contributors and Ubuntu users are going to appreciate it. (Docs can be found here.)

Read more
Daniel Holbach

New contributors who don’t have upload rights to Ubuntu yet get their code reviewed and their packages uploaded by Ubuntu developers. This process is called “sponsoring” and our current process has been in place since pretty much forever. It has even gotten easier over time, so new branches or patches show up on our review queue.

Two years ago when we were struggling with getting code reviewed, we put in place “patch pilots”, a great concept we borrowed from the Bazaar team. We set up a monthly schedule and Canonical provided 4 hours per month per engineer with upload rights to make sure code gets reviewed. This has helped a lot.

Getting closer to the 12.04 release, it looks like we need to put some extra effort in and need some help.

Sponsoring Stats

That’s right we have been hovering around 50 for a while now, dealt with many incoming new requests, but still we don’t get down to 0. If you can review code, please help out.

We all are interested in getting new developers on board. This only works if we review each other’s work, gain each other’s trust and give each other advice.

The Sponsorship queue is where a lot of exchange about this happens and where knowledge is passed on. Help out by reviewing today and help grow our community this way.

This is one of the most valuable contributions to Ubuntu! This matters to all of us.

If you want to see at once glance how we are doing and who’s all helping out, head over to our one glance sponsoring page. (Patches to make it look more Ubuntu-y are very welcome!)

Check the instructions for code review (with lots of tips and tricks) and get your name on the page as well!

Read more