Archive for June, 2006

Why code reviews are good for you

Over the years, I have had the opportunity to work with various code review systems, and I have come to believe that one particular approach works better than the other.  Here is what I’ve learned.

There are basically three ways to approach code reviews:

  • No code review.  Everybody checks in code liberally, and while everyone is welcome to examine change lists as they please, there is no incentive and no obligations to do so.
     
  • Non-blocking code reviews.  When code is submitted, a reviewer is associated to the change list and they are expected to review the changes at some point, but this review does not block the submission of the code, which gets incorporated in the depot right away.
     
  • Blocking code reviews.  In this scenario, submitters associate a reviewer to their change list but that change list does not get submitted until the reviewer gives their approval.

Let’s get rid of the first case quickly:  I strongly believe that projects that work without any peer review will end up with code of significantly worse quality, regardless of how talented or experienced the developers are.  It doesn’t matter how good you are, you can’t produce top quality code all the time.  We all get sloppy at time, and code reviews are here to address these times.

When the code reviews are blocking, the committer is not allowed to check in their code until the reviewer has responded with a positive comment.

When they are not blocking, the code gets checked in right away but the reviewer still receives an email giving all the details about the code so that it can be reviewed later (I suggest setting up a nag system that will remind lazy reviewers to do their reviews at least once a week).

In my experience, the only system that works is non-blocking code reviews, but before I go into details, let’s examine exactly what code reviews do, because I found that very often, the perceived benefits are different from the real benefits.

Here are a few facts about code reviews:

  • Code reviews don’t reveal critical flaws in your code.

    This is disappointing, but it’s the sad truth.  As the system becomes complex, so do the change lists, and you won’t often hear a reviewer say "If the servlet receives more than twenty connections, you will have a deadlock between A.java:723 and B.java:1410".

    More likely, reviewers comments will be cosmetic ("Please rename mI to midIndex, it makes the intent clearer") or they will request more comments.
     

  • Code reviews do keep you honest.  They force you to take some extra time to make your code more readable, because you know for sure that someone will be reading it with a critical eye very soon.

I found that proponents of blocking code reviews usually feel very strongly that anything more liberal is bound to be the cause for a significant drop in quality as well.  The argument often goes like "If reviewers do not feel a strong obligation to review the code, they will slack and the review won’t be done".

First of all, my experience tells me that this is incorrect.  I found that even in the most liberal non-blocking code review systems, reviewers were very diligent with their reviews and they usually got done in a fairly short amount of time (less than a week, and usually within a couple of days).

But what’s inherently flawed with this assumption is that it fails to recognize that both blocking and non-blocking code reviews rely on the honor system, so saying that one will work while the other won’t doesn’t make sense.

In the blocking code review scenario, when you submit your code, you rely on the reviewer realizing that you are now unable to perform any additional work (or at a considerable price, see below) until the review comes in.  Despite this pressure, it’s not uncommon to see reviewers take several days before reviewing code, because they are just swamped and they also know that the committer is taking measures in order not to stay inactive while she’s waiting on the review.

Problems with blocking code reviews

I haven’t had good experiences in environments that mandate blocking code reviews.  Here are the main drawbacks to such a system:

  • While they wait for the review, developers are stuck and have to work on something else or put in place a system of patching, which they will have to resolve next time they do a sync and try to commit their approved code changes.
     
  • Developers know that they will be stuck as soon as they commit, so they accumulate huge change lists in order to keep working and "stay in the zone".
     
  • These huge changes lists become problematic for reviewers who decide to not take a look at the change list until they can carve out a significant portion of time, which delays the review even further, therefore creating a vicious cycle.

While non-blocking code reviews work better

I strongly believe that non-blocking code reviews address all the problems listed above and come with their own set of good properties.  Here are some of them:

  • Being a reviewer when the review is not blocking works well, because a conscientious developer will be aware that reviewing the code as soon as possible is part of their job, just like writing tests or comments.
     
  • I am much more likely to take an extra ten seconds to write a comment if I know for sure that somebody will be reading my code, and possibly comment on it (which is why I said above that "code reviews keep you honest").  Conversely, I have very often noticed that code that was never reviewed by a peer is usually harder to follow because it is more sloppy and less commented.
     
  • Reviewing code written by colleagues forces me to a daily and healthy exercise.  I have to change my mindset and get familiar with idioms and styles that are not mine.

What about pair programming?

Pair programming is definitely one way to do code review, but it suffers from a few limitations that, in my opinion, make it a less desirable option than non-blocking code reviews:

  • Pair programming only catches micro-errors.  Contrast this with non-blocking code reviews that expose your reviewer to an entire change list, which will force them to see a bigger picture than they would have if they were pair programming with you.
     
  • Pair programming naturally forces the two participants to converge toward the same thinking.  Once you both decide that you need a class A, you will naturally agree that it implies the creation of classes B, C and D, and after a while, you have a "design of a single mind".
     
  • Pair programming can also take longer.  The usual argument to defend pair programming is to say that programmer A can do task X in three minutes and task Y in twenty-five minutes, while the times will be reversed for programmer B.  Put these two programmers together and they’ll be able to complete tasks X and tasks Y in three + three minutes.

    This is nice, but quite idealistic.  In reality, the outcome of pairing A and B can have vastly varying results, such as a task that would take three minutes by programmer A alone suddenly takes thirty minutes to accomplish with B in the picture.  Or A and B spending a lot of time arguing about the "right" way to solve the problem, while ultimately, there are more than one right way and A would have solved the problem much faster on their own.

Anyway, the point of this post is not to criticize Pair Programming but simply to make it obvious to everyone that the full impact of Pair Programming hasn’t been studied very well so far, and that it’s not a very good substitute to simple peer code reviews.

Finally, Brian Slesinsky wrote an interesting entry praising Pair Programming and criticizing code reviews.  Take a look if you want to get a counter-opinion to mine, but in general, I find that Brian’s arguments are a demonstration that blocking code reviews are not optimal much more than he demonstrates that pair programming is the right solution.  I think that non-blocking code reviews have all the characteristics of a happy medium between these two radical approaches.

What about code ownership?

Shared code ownership is vastly over hyped.

Shared code ownership is presented as a desirable consequence of Pair Programming and it simply means that since at all times, there has always been at least two people working on the same region of code, the entire team has a much better knowledge of the code base and that everyone can therefore step in at any time to fix a problem even in the absence of a key member.

First of all, reasonable shared code ownership can be reached with peer reviews.  It’s pretty easy to see why:  when you spend some time reviewing a teammate’s code, you gain a very familiar knowledge of it, and it won’t take you long to dive back in if you need to.

But more generally, shared code ownership is simply unnecessary if the code has been written by competent developers under careful code reviews.  It is very common for me (at least once a week) to stumble upon an area of code that I’ve never seen before, and I usually have no problem finding my way around it if the developer has followed some simple rules (commenting, naming the classes, methods and variables appropriately, using design patterns, etc…).  Code reviews will buy you a micro-knowledge of the code, but I find that I usually don’t need to know the code in so much details most of the time.

Finally, if you need a proof that code ownership is vastly over hyped, look no further than open source projects.  In typical open source projects, reviews are not mandatory and everyone can check in code at their will.  New developers come and go and they don’t seem to have much problem learning the new code base, assuming that’s it’s been commented and written with good design principles.

Conclusion

If your organization is not mandating code reviews, you should reconsider this decision and, ideally, opt for non-blocking code reviews.  There are various other ways to implement code reviews that I did not cover in this document, but whatever you choose, make sure that members on your team regularly get exposed to code coming from other members, even if superficially.

How about yourself?  Are you doing code reviews in your company?

 

Agile people still don’t get it

I just attended Test-Driven Development presentation which represents everything that is wrong about the way Agile advocates are trying to evangelize their practices.  I don’t have anything against the presenter in particular, but it’s really time for Agilists to rethink the way they communicate with the real world.

Here are a few comments on his presentation.

One of the first slides that deeply troubled me claimed the following:

  • Tests are (executable) specs.
  • If it’s not testable, it’s useless.

First of all, tests are not specs.  Not even close.  Somebody in the audience was quick to give a counter-example to this absurd claim by using a numeric example ("how do you specify an exponentiation function with a test?") but my objection to this claim is much broader than that.  Relying on tests as a design specification is lazy and unprofessional because you are only testing a very small portion of the solution space of your application (and of course, your tests can have bugs).  Tests also fall extremely short of having the expressiveness needed to articulate the subtle shades that a real specification need to cover to be effective.

This claim is part of a broader and more disturbing general Agilist attitude that is usually articulated like "Your code is your spec", along with some of its ridiculous corollaries such as "Documentation gets out of date, code never does".

Anyone who claims this has never worked on a real-world project.  And I’m setting the bar fairly low for such a project:  more than five developers and more than 50,000 lines of code.  Try to bring on board new developers on this project and see how fast they come up to speed if all they have to understand the code base is… well, just code.  And tests.

I am currently getting acquainted with a brand new project that is not even very big, and while I understand Java fairly well, there is no doubt in my mind that for ten minutes I spend trying to understand how a certain part of the application works, a five-line comment would have given me this knowledge in ten seconds.

The second claim, "If it’s not testable, it’s useless" is equally ludicrous and a guarantee that at this point, the audience you are talking to is already looking at you as a crackpot.

Software is shipped with untested parts every day, and just because it’s not entirely tested doesn’t mean it’s bad software or that the untested parts are "useless".

Agilists just don’t understand the meaning of calculated risk.

Early in the development cycle, it’s perfectly acceptable to go for a policy of "zero bugs" and "100% tests".  But as the deadline looms, these choices need to be reconsidered all the time and evaluated while keeping a close eye of the final goal.  Very often, Agilists simply forget that their job is to produce software that satisfies customers, not software that meets some golden software engineering scale.

Anyway, let’s go back to the presentation, which then proceeded with the implementation of a Stack class with TDD.  Before spending thirty minutes on a live demo of the implementation of a Stack class (are you impressed yet?), the presenter warned the increasingly impatient audience that they should "not pay too much attention to the Stack example itself but to the technique".

And that’s exactly the wrong thing to do.

Look, we "get" TDD.  We understand it.  Frankly, it takes all of ten minutes to explain Test-Driven Development to a developer who’s never heard of it:  "Write a test that fails and doesn’t compile.  Make it compile.  Then make it pass.  Repeat".

The hard part is applying it to the real world, and showing the implementation of a Stack will soon have everyone leave the room with the thought "Cute, but useless.  Now let’s go back to work". 

It was even worse than that, actually:  The presenter kept taking suggestions from the crowd but he declined all those that didn’t fit in the neat script that he had in hands at all times.  These suggestions were good, by the way: 

"What should we test now?"

"How about:  if we pop an empty stack, we get an exception"

<a bit embarrassed> "Mmh, no, let’s not do that" <hand waving, look down at notes and proceeds while happily ignoring the other raised hands>

To be honest, I am becoming quite suspicious of Agile practices for that reason:  all the presentations I have attended and books that I have read are always using toy implementations as examples.  Stack, List, Money, Bowling…  enough already!  Let’s talk about TDD for code that interacts with clustered databases on laggy connections built on 500,000 lines of code that was never designed to be tested in the first place (and:  yes, I read Michael Feathers’ book, it has some good and some bad, but it’s not germane to Java and TDD so I won’t expand on it here).

And please, avoid smug and useless answers such as:

"A lot of the classes I have to test are hard to isolate, do you have any advice regarding mocks?"

"Well, if you had started with TDD in the first place, you wouldn’t be having this problem today".

Fundamentally, I am disturbed by the Agilists’ dishonesty when it comes to presenting their arguments.  They offer you all these nice ideas such as Test-Driven Development and Pair Programming but they never — ever — disclose the risks and the downsides.  To them, Agility is a silver bullet that is applicable in all cases with no compromises.

The truth is that these practices come at a price, and for a lot of organizations, the price gets high very quickly.  Agile development will never go far if its proponents keep ignoring these organizations and make condescending comments to its members.

I like Test-Driven Development.  I really do, and I’m fortunate enough to work on a project that lets me use TDD most of the time.  But the truth is:  at times, I don’t do TDD because implementing a feature quickly is more important than a fuzzy feeling.  And I’m also aware that TestNG is an open source project with less than five developers, all of them on the bleeding edge and aware of the latest advances in software engineering.

And this is my main beef with Agilists:  I strongly suspect that most of them are spending their time on open source projects with like-minded fellows, but none of them have any experience what companies whose survival depends on shipping software have to go through to organize huge code bases growing thousands of lines of code every day under the combined push of hundreds of a developers, all with their personal background, education and bias.

So here is my advice to Agilists:  get real now, or you will become irrelevant soon.

Update: four years after I posted this article, reddit picked it up.

Best sci-fi books I’ve read in a while



It’s not very often, but once in a while, I come across books that are so great
that I have to share them with everyone.

This time, it’s a science fiction
trilogy written by a British author named Alastair Reynolds.  The books
are:

I would qualify his science between "hard" (e.g. Arthur C. Clarke) and pure
fantasy (e.g. Perry Rhodan).  A lot of the technology used in these books
are rooted in the latest physics theories (including brane worlds!) but from
there, he comes up with some very creative and innovative ideas that help carry
the plot along.

Reynolds’ writing is deep and detailed (as with most British
authors, I found) and his storytelling arcs over the three books with a lot of
cleverness and a sense of reflection that has made me like science fiction
literature again.  I won’t disclose anything about these books except for
the general idea:  if there is intelligent life out there, how come we
never encountered it?

Reynolds has his own interpretation to this paradox and he shares it with us
through colorful and complex characters thrown into chaotic events that advance
ineluctably with sophisticated twists and relentless action.

This is a saga with a background theme that is slowly emerging through the books
and that he advances by telling multiple unrelated stories at first but which
slowly converge toward their climax (not unlike Hyperion or Perry Rhodan). 
Some of these unrelated threads are also told at different periods of time,
which reinforces the feeling of a grand odyssey that is rooted deep in our past
and that has sweeping and possibly devastating consequences for the future of the
human race.

Page turners that makes you think.