Wednesday, July 26, 2006

Good Code and Examples

Why are so many examples in books so poor? For example, take this one from page 254 of "Java In A Nutshell" 5th edition:

//Open a file for read/write ("rw") access
RandomAccessFile f = new RandomAccessFile(datafile, "rw");
f.seek(100); //Move to byte 100 of the file
byte[] data=new byte[100]; //Create a buffer to hold the data
f.read(data); //Read 100 bytes from the file
int i = f.readInt(); //Read a 4-byte integer from the file
f.seek(100); //Move back to byte 100
f.writeInt(i); //Write the integer first
f.write(data); //Then write the 100 bytes
f.close(); //Close the file when done with it

"What's so wrong with this?!", you ask. It's chock full of baby sins. It's important that beginners will copy the example and try it on their own. Sometimes this code is tweaked into production code. Just think someone at 3am is probably answering a pager because of a bad code example somehwhere in the universe. Why not take the opportunity as teachers to show off exemplery code?

So, what are the sins? Right off the bat, poor variable names. It should be an abomination to have single letter variable names. We're in the 21st century finally! Variable names don't take up much more space. Why not be intention revealing?

The next sin is that the example is not useful. It would be nice to have a small example of what a random access file is good for.

The magic values is the next killer. Why not make constants? Production code should never have magic values neither should your examples.

The final sin is the worst. No finally or any exception handling code on the file close. You need to always make sure you leave the file in a good state (i.e. not open). There's several ways to do that, but at least make sure your examples ensure they close themselves.

Here's how I would have done the same example:

private static final String READ_WRITE_ACCESS="rw";
private static final int TOTAL_RECORD_LOCATION=100;

public void addAmountToTotalAndSave(int amountInCents, String fileName) throws IOException {
RandomAccessFile totalFile = new RandomAccessFile(fileName, READ_WRITE_ACCESS);
try {
//go to total record location
totalFile.seek(TOTAL_RECORD_LOCATION);
int previousTotalAmountInCents = totalFile.readInt();
int newTotalAmountInCents = previousTotalAmountInCents + amountInCents;
//reset position to total record location so that we can write new total
totalFile.seek(TOTAL_RECORD_LOCATION);
totalFile.writeInt(newTotalAmountInCents);
} finally {
totalFile.close();
}
}

I could have broken this code out more, but I wanted something concise to show RandomAccessFile without much fluff. But, I also showed good coding practices. It's also a simple example that beginners could easily wrap their head around. Remember we are the teachers and it is our job to always show great code.

No comments: