Coding Defensively

I have been involved in a code review for the past few days. Time and time again I have come across code that fits into the “if you know something will never happen, it most certainly will” category of development. Take a look at this example:

List users = session.find("select u from User u where u.loginName = ?", ... );
if(users.size() > 0)  {
...
return true;
}
return false;

This probably looks like 99% of the code out there. The problem is that you’re only concerned with the case where the size is equal to one. The case where the size is greater than one is undefined.

I know, you’re thinking to yourself: “But that will never happen since I have unique constraints on my primary keys. The entry app will puke when it attempts to enter more than one row.” Never say never. A few years ago I was working on an application with the same constraints. In order to speed up and allow for an ETL operation that the DBA was doing, he disabled the all of the constraints and forgot to re-enable them. Rather than having logging in place that would have caught this error immediately, a few weeks went by without anyone noticing. Needless to say, it took a few weeks to clean up the resulting mess. Oh, did I forget to mention that this was a production database?

A more sensible and defensive coding strategy would be:

List users = session.find("select u from User u where u.loginName = ?", ... );
// NOTE:  the size of users is expected to be [0, 1]
final int usersSize = users.size();
// if the size of users is greater than one, log an error
// but continue as this is not fatal
if(usersSize > 1) {
// log something
...
} /* else -- users size is not greater than 1 */
// there is at least one user.  The first user will always
// be used.
// NOTE:  more than one user may be present at this time.
//        This case can be safely ignored at this point.
if(usersSize > 0)  {
...
return true;
} else if(usersSize == 0) {
return false;
} else { // usersSize is less than zero
// this is an error that cannot be attributed to this code
// in any way.
throw new DeveloperException("<some helpful text>");
}

It is up to your particular application guidelines to determine whether or not the exception cases should be immediately bubbled out to the user as errors. Personally, I am not a big fan of asserts in Java 1.4 in web apps due to the problems of effective exception handling. (Here is a good starting point for the problems associated with Java’s exceptions in general.) Let me stress that how you handle the cases that are unexpected is not as important as getting into the habit of thinking about them and notifying someone somehow when they occur. As long as you’re consistent in dealing with these cases the time spent up front will save you precious time in the end.

Advertisements

One comment

  1. Pingback: Code Comments « rgrzywinski


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s