Categories
Architecture Funny

Exceptionally Bad Exception Abuse

Nick Hodges' Coding In DelphiI was reading Nick HodgesCoding in Delphi again last night (insomnia). In chapter 1 on Exception Handling he opens talking about how not to use exceptions. It got me thinking about some of the atrocious exception usage I’ve seen. I thought I would share a few examples beyond the usual exception sins.

The Try / Except / Raise / Free Pattern

The documentation on the Try / Except pattern says that all the code after the Except block gets ran because the exception is handled. Additionally you can call Raise during the handler to send the exception further up the call stack.

I had just started a new job and was sitting with one of the senior developers to fix some bugs and get to know the codebase. He was creating a local object and so needed to free it. This is the pattern he used:

sl := TStringList.Create;
try
  // using sl
except
  raise;
end;
sl.Free;

I looked at the code for a minute quizzically, and then asked why he did that. He explained that everything after the except always gets ran, so he can be sure the memory is freed, but still let the exception bubble up to the global exception handler so it is logged. I told him that I was pretty sure calling raise in the except block prevented the code after the block from being ran. He assured me he had been writing code like this for a long time, and it always works.

I thought about it a bit more and then suggested we test the code. So we made a little test application with a message after the except block, and forced it to raise an exception, and sure enough, the message didn’t show. (Always test your theories and patterns with test code!)

There was a look of panic on his face. He’d been using this pattern for years, and it was all over multiple codebases. He claimed that the documentation must be wrong or it changed in a recent update, but I tried to assure him that is was clear and always worked that way, he just misunderstood.

Log ’em All and Let Support Sort ’em Out

This was using an early version of C# and WinForms.NET in Visual Studio. I took over a project and was having a hard time tracking down some odd behavior. Some code just wasn’t being ran, and there was no error messages. This was a project that was really, really early in the development process, and my first large scale project in C#, and I was getting really frustrated.

I finally discovered that an exception was being raised, but it was never showing up as an error. Even if the debugger was attached there was no exception message. The program just kept on running as if nothing happened, but the operation aborts. I started to question .NET’s ability to handle exceptions.

I dug and dug and finally figured out that the previous developer had setup a global exception handling and logging system that captured all the exceptions and logged them into the eventual support database with a detailed call stack. IIRC WinForms apps close automatically if there is an unhandled exception, so having an exception handler is a good idea, but silently logging them seems questionable. And then since there was an exception logging system in place the developer set the project options to have the debugger ignore all exceptions.

The result is an app running completely silent of exceptions, even while developing, but at least support will have a lot of debug information. I talked to the original developer, and he thought it was a brilliant idea, and said it is the first thing he does in all his projects. He just looks in the support database to see if things are working. His logic being users don’t want to see error messages. Not sure how hiding all the error messages is a good idea, especially hiding them from the developer during development.

Exceptions as Robust Return Objects

I was maintaining some code where a previous developer built this system where custom Exception were used to return a complex type system. He built this impressive hierarchy of exception classes, and then only certain types of exceptions were handled at different levels, where the data was needed. So instead of methods returning a result, or modifying a passed value, they all just raised different exceptions, with the properties set to different values.

There are really two issues here in my opinion. The first is raising and handling exceptions is rather expensive. A better option would have been to just create plain old objects (or even interfaced objects) and return them as results. You can still examine their types and decide when to handle them. You would just need to be aware of managing the object lifecycle (the automatic lifecycle handling was his justification for using exceptions).

The second issue is this was just one subsystem of a larger project. It was only this subsystem that used this bizarre data management system. So when you were fixing a bug here there was a mental tax of wrapping your brain around the different architecture. As soon as you figure it all out, fix the bug, and move on to other code you go back to the normal way of thinking. Again paying the mental tax of forgetting the atrocity you just witnessed.

I really believe that in a large project there should be some consistency in how the pieces are architected unless there is a compelling reason to do things different in one subsystem. That makes it easier for developers to work in different parts of the system.

So what are the craziest abuses of exceptions and exception handling you’ve seen?

By the way, Nick has a new book out on Dependency Injection in Delphi. You should pick up a copy.Nick Hodges' Dependency Injection In Delphi

11 replies on “Exceptionally Bad Exception Abuse”

// Make sure you catch any exception and throw away
// all exception information and any chance of logging a
// call stack at a higher level!
try

except
on E: Exception do
raise Exception.Create(e.Message);
end;

// Cause a very hard to find access violation!
try

except
on E: Exception do
begin

raise E;
end;
end;

It might be the weather, but I don’t understand your second example with the “access violation”. Can you please elaborate a bit?

If you want to re-raise an exception from within the exception block, then you use just “raise” if you do “raise e” like Natalie showed then it raises it a second time (starting a new lifecycle), but the exception handler frees the memory of the exception object, so then the new lifecycle has an invalid object, which causes an AV.

In such case and to ensure both a good exception handling flexibility and at the same time ensure cleansing operations such as freeing or closing resources use a try except embedded within a try finally.
Try
Try
Create objects;
Do something;
Except
Handle exception;
End; // try except
Finally
Free resources and objects;
End;// try finally

I once knew a developer who prided himself on never reading a single book on programming. He called himself a senior software developer and claimed to be a Delphi expert. I came across the following code he had written

Dataset.First;
try
DoStuffWithDataset;
except
Dataset.Next;
end;

I asked him why he had written this and he said, “Because we wanted to be sure that every record was processed.”

When I told him this code would not work that way, he said he’d been programming Delphi for ten years and knew how exceptions worked. It turns out the codebase was littered with this kind of nonsense and he kept putting in more than I had the time to fix. At meetings where bugs were discussed, he and his allies kept blaming the previous programmers for the continued buggy nature of the code, but every time I fixed a bug, it was something he had written. After a few months, we all got fired because the client was fed up with the buggy code.

Yes, never use `raise E;` on a caught exception! Either 1) use `raise;` to re-raise the current exception as-is (the current exception handler will release ownership, and the next exception handler will acquire ownership), 2) use `raise ExceptionType.Create(…);` to raise a completely new exception while freeing the current exception, or 3) use `Exception.RaiseOuterException()` to raise a completely new exception that “steals” the current exception and puts it in the InnerException property (the new exception will free the previous exception when the new exception is itself freed).

@Adrian… it’s been a while since I used Indy so this example might not be 100% correct but they’re probably referring to something like:

http://ftp.connect
http://ftp.put
http://ftp.quit <– raises exception 'connection closed gracefully'

Kind of aggravating having to account for those sorts of exceptions, IMO. An aside, Remy's comments on newsgroups and stack overflow have helped me countless times over the years – many thanks, Remy.

Comments are closed.