Positronic Vibrations

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.

Comments

Blaine,

I don't read your blog as often as I'd like, but whenever I do it gets me thinking.

I cut my teeth on the first edition of "Java in a Nutshell" and agree many of the code examples are horrible. For some unfathomable reason, most Java books make little or no attempt to teach OO concepts, choosing instead to focus on syntax and the API.

The problem with correcting code is that your corrections are open for correction :) In fixing the "baby sins" you've created, IMHO, a more egregious one. public methods with filname parameters is generally a Bad Idea as it allows a careless coder to corrupt any file of his choosing.

You've also created some minor baby sins (embryo sins?) yourself. I believe redundancy does not necessarily lead to clarity. The repetitive "InCents" while well intended, gets downright annoying. I only need to be told once that we're dealing in cents. When you switch to another unit measure, let me know, otherwise it should be safe to assume nothing has changed. Make cents, er, sense? By renaming the method to addCentsToPurse, it pretty clear that cents is the order of the day.

Your constant and variable names are a little too "techie." I've lately come to understand that if code can be read by a non-coder, coders will be able to read it all the faster. seek(TOTAL_AMOUNT) is less technically accurate than seek(RECORD_POSITION) but it more clearly indicates what we expect to find at that location.

So, here's my version (hoping the format looks ok):

private static final String PURSE = "blah";
private static final String READ_AND_WRITE = "rw";
private static final int TOTAL_AMOUNT = 100;

public void addCentsToPurse(int cents) throws IOException {
RandomAccessFile purse = new RandomAccessFile(PURSE, READ_AND_WRITE);
try {
purse.seek(TOTAL_AMOUNT);
int centsInPurse = purse.readInt();
int total = centsInPurse + cents;
purse.seek(TOTAL_AMOUNT);
purse.writeInt(total);
} finally {
purse.close();
}
}

As a final note, I'm always tempted to combine multiple lines, as in:

purse.writeInt(centsInPurse + cents);

While this is a trivial example, it does help with debugging to use a new variable and multiple lines.

Jeff

By Jeff, at 5:05 AM

Positronic Vibrations

Wednesday, July 26, 2006

Good Code and Examples

Comments