Adrien Kunysz, Sat, 21 Sep 2013 14:10:28 CEST
I have relatively little professional experience reviewing code but I like to think I have some decent skills in hunting bugs.
Tracking down bugs is fun but often frustrating. In general when you are trying to figure out why it's broken, there is a pressure to fix the problem ASAP. You end up going trough various layers of applications, libraries and kernel, usually noticing many broken or ugly things that are not relevant to the actual problem at hand, trying to make a mental note to fix it later. Of course, by the time you found and fixed (or worked around) the actual bug, you already forgot about the other problems and there is no way you are going back into that code as long as it's "working". Especially when the code you have been through is being maintained (or not) by different people or organisations.
On the other hand, in my experience, code review is much more fun. You can focus on a specific and usually short portion of code. There is no immediate problem to fix. You are just asked to criticise a piece of code and the more problems you find, the better it is. Well, kind of, as it's often better if you know what you are supposed to look for otherwise you can practically find something wrong with every single non blank line (and then sometimes with blank lines too). But best of all, you are generally not the one supposed to fix the potential problems you find and if you can't figure out the code you can just claim it's too badly written. This is actually a great activity for a pedantic sociopath like me.
Qu'on me donne six lignes écrites de la main du plus honnête homme, j'y trouverai de quoi le faire pendre. -- Cardinal Richelieu, circa 1600
Qu'on me donne cent lignes de C écrites de la main du plus paranoïaque des programmeurs, j'y trouverai des bugs. -- me, circa 2007
I have found that performing that activity on paper listings while at the pub is good fun and much more entertaining (to me) than the typical pub quizzes you get in England (and other places but my experience in this has mostly been in England).
A couple of years ago at a certain pub-located event, someone gave a talk on randomness. Not a random talk mind you but a talk on how not to misuse randomly generated numbers. Shortly before the talk, paper copies of nine programs in six languages (C, Java, C#, Perl, C++ and PHP if you need to know) were distributed. This is one I got:
#!/usr/local/bin/perl
# Example 4: Load Balancer
# Copyright (C) 2011 Cigital, Inc.
#
# Paco Hope <paco@cigital.com>
#
# Load balancer: given an array of valid back-end hosts,
# randomly pick one to assign to the current connection
# request.
use Cigital::HWRNG;
sub getHost() {
my @hosts = shift; # Array of hosts comes in as parameter
my $numhosts = $#hosts;
die "Can't do more than 256 hosts!" if $numhosts > 256;
do {
# Fetch single byte until we get one in range.
$rand = $HWRNG->nextByte();
} while ( $rand >= $numhosts );
# Return the host corresponding to the number we picked.
return $hosts[$rand] ;
}
I quickly found five problems with that snippet of code, only one of which is really relevant to mishandling of random numbers (no, I don't count "it's Perl" as a problem). Some beers later, the talk started and at some point the speaker announced that seven of the programs that were distributed were broken while the remaining two were, to the best of his knowledge, OK as far as proper handling of randomness is concerned. We were then given a few minutes to try to figure it out on our own. Some paper shuffling later I end up with this one:
#!/usr/local/bin/perl
# Example 6: Bing Board
# Copyright (C) 2011 Cigital, Inc.
#
# Paco Hope <paco@cigital.com>
#
# Bingo board implementation. There are 5 rows and 5 columns labeled
# B, I, N, G, O. It looks like this:
#
# | B | I | N | G | O |
# ===+===+===+===+===+===+
# B | A : B : C : D : E |
# ---+---+---+---+---+---+
# I | F : G : H : I : J |
# ---+---+---+---+---+---+
# N | K : L : M : N : O |
# ---+---+---+---+---+---+
# G | P : Q : R : S : T |
# ---+---+---+---+---+---+
# O | U : V : W : X : Y |
# ---+---+---+---+---+---+
#
# 25 numeric values are drawn to make a board (labeled A-Y in the boxes).
# Each draw consists of a letter (BINGO) followed by a number. If the player
# has that number in either the column or row of that letter, he marks it
# off. E.g., if the draw is "12", then the player marks off any box where
# the number 12 appears. If he gets marked squares in a straight horizontal
# or vertical line, he wins. Ideally each board is randomly made and each
# draw is random so the winner is essentially random.
use Cigital::HWRNG;
# Return a series of 25 unique integers drawn from 1-99 inclusive. These
# will be inserted into the board in positions A-Y as shown above.
sub newBingoBoardNumbers() {
my $rand;
# hash table to hold selected values. Makes it quick and easy to
# figure out if we've already tried to pick this number.
my %selected = {};
for( my $i = 0; $i < 25 ; $i++ ) {
do {
$rand = $HWRNG->nextByte();
# Fetch single bytes until we get one in range.
# Check to be sure it's not one we've already selected, too.
} while( $rand > 98 and (! $selected{$rand + 1} );
# mark it selected ($rand+1 because we're pulling 0..98 and
# we want 1..99
$selected{$rand + 1} = 1;
}
# Return the hash table with all the unique numbers we picked.
return %selected ;
}
# call the other function and build a bingo board from the selected
# numbers
sub makeBingoBoard() {
my %selected = newBingoBoardNumbers();
# The hash is trivial, with each entry being something like
# 98 -> 1, 43 -> 1, etc. That is, the "key" is the number we'll use
# on the bingo board and the "value" is just a 1, so that it has an
# entry in the table.
# getting the "keys" of the hash gets us the numbers.
my @nums = keys( %selected );
for( $row = 0; $row < 5; $row++ ) {
for( $col = 0; $col < 5; $col++ ) {
# pop a number of the array of numbers
$board[$row][$col] = pop( @nums );
}
}
# ... do other stuff to print the board ...
}
I wasted about 30 seconds trying to make sense of the overlong top comment before giving up and going through the code itself (I am still not sure how a bingo board is supposed to work and I am not really interested). Once in the code I quickly found four potential problems, three of which are similar to things found in example 4 above and not really relevant (and I noticed a completely irrelevant fifth problem just now as I type this) while the fourth problem was relevant to the talk.
When the talk resumed, it went more or less like this:
<speaker> example 4 is correct
<speaker> example 6 is a bit complicated
<Krunch> it's broken because of XXX
<speaker> that's right, you are only the second person I show this to have figured it out
*applauses*
<Krunch> and I think example 4 is broken too
<speaker> oh hum
*laughters*
After having been able to actually check by running perl (eh, I only "proved" them to be broken, I hadn't actually checked until then), I can confirm both examples are broken. Finding exactly how is left as an exercise for the reader although I might give hints by mail or IRC if you are that desperate.