Today at work, I came across some interesting code. I looked at it for quite some time before it occurred to me what was wrong with it. Below is a simplified synopsis of the code.
The following code uses a database query I will call Q and a boolean function I will call F.
function F(): boolean;
begin
try
if (Q.Active) then
begin
// do some stuff
result := true;
end
else
begin
result := false;
ErrorMessage := 'No results found';
end;
except
on e: Exception do
begin
Q.Close();
Q.Open();
F();
end;
end;
end;
//
// Main method
//
Q.Open();
F();
If you can’t see the problem, it was really twofold. First, god help you if an exception gets thrown. If so, you could go into an infinite recursion loop if the issue was an error occuring on the database side. Second, if the connection cannot be opened, the function would return false and tell the user that there were no results in the database. In truth, there very well could be a million results and the function would effectively be lying. Although the code isn’t horribly written stylewise, I cannot believe someone would do something like this. It absolutely amazes me.
The first problem that I have with this code is that, IMO, you should never retry something when an exception is thrown. You should recover, clean up, and report the error to the user. If you truly wish to try over, you should do that from the calling location and not inside the exception. The second problem I have with the code is that the function is returning a result based on whether the database can be contacted, not the results of the query. I can see the function returning false when it fails, but the error message should tell the user that the database wasn’t open, or couldn’t be connected to, or was closed; anything other than flat out lying to the user. geesh.
Leave a Reply
ABOUT
barbecueguy.com