Why use exception handling in apparently "safe" code?

Question

Please, may somebody explain me, what can raise an exception in this code?

function CreateBibleNames: TStrings;
begin
  Result := TStringList.Create;
  try
    Result.Add('Adam');
    Result.Add('Eva');
    Result.Add('Kain');
    Result.Add('Abel');
  except
    Result.Free;
    raise;
  end;      
end;

Since I use delphi I have used exception handling perhaps once. I consider the code above to be written by a skillfull programmer and I do not think the exceptions are redundant. But still, using exception handling in this concept remains a mystery for me. It seems to be a safe code (without try except end). I have seen many times similar code snippets like this, that's why there is probably a good reason to write it this way in spite of my experience, that did not prove it's necessity.

Moreover when something fails, I get exception description....

Thanx

Answer 1

The most obvious exception would be whatever Delphi uses for an OutOfMemoryException. If there isn't enough memory to add a couple strings to a list, free the whole list and re-throw the exception.

That seems like overly defensive programming for making a list of predetermined names -- generally, if you get one exception due to being out of memory, your whole application is hosed.

Answer 2

The only potential reason for an exception I can see is an OutOfMemory exception and if this is the case - all bets are off anyway. In my opinion this is an example of abusive use of a valid concept.

Excessive and unjustified use of try except clutters the code and makes it more difficult to read

Answer 3

As an interesting side note, some of the authors of Java now consider the decision to require the catching of exceptions in virtually all cases to be a mistake. That's because most exceptions really result in the program being killed, so you might as well let the runtime system kill the program and print a stack trace, rather than forcing the programmer to catch the exception and ... well, just print something and kill the program.

Answer 4

Without the source for TStringList, or authoritative documentation, you can't really know. Just for illustrative purposes, let's suppose that TStringList is written such that when memory gets tight, it starts swapping portions of the list to and from disk, allowing you to manage a list as large as your disk. Now the caller is also susceptible to the gamut of I/O errors: out of disk space, bad block encountered, network timeout, etc.

Is handling such exceptions overkill? Judgement call based upon scenario. We could ask NASA's programmers what they think.

Answer 5

It's a good habit to be in when returning newly constructed objects from functions. In a more complex scenario than a string list, some invariant might be broken or other error occur, and hence an exception, and in that case you want the return value freed. Rather than have to examine every possible situation when this may occur, it's generally a good practice to have a single pattern for returning newly constructed objects and follow it everywhere.

That way, when the implementation of the objects you're constructing changes in the future, you're still safe if they happen to throw an exception.

Answer 6

Okay, that code is strange, I agree, and I totally understand why it got written that way. But it was written that way because the premise underlying the code is wrong. The fact that the construct seems strange should be a "code smell", and should tell you that something might not be getting done the best way possible.

First, here's why the unusual construct in the try...except block. The function creates a TStringList, but if something goes wrong in the course of filling it, then the TStringList that was created will be "lost" out on the heap and will be a memory leak. So the original programmer was defensive and made sure that if an exception occurred, the TStringList would be freed and then the exception would get raised again.

Now here is the "bad premise" part. The function is returning an instance of TStrings. This isn't always the best way to go about this. Returning an instance of an object like that begs the question "Who is going to dispose of this thing I've created?". It creates a situation where it might be easy -- on the calling side -- to forget that a TStrings instance has been allocated.

A "Better Practice" here is to have the function take a TStrings as a parameter, and then fill in the existing instance. This way, there is no doubt about who owns the instance (the caller) and thus who should manage its lifetime.

So, the function becomes a procedure and might look like this:

procedure CreateBibleNames(aStrings: TStrings);
begin
  if aStrings <> nil then
  begin
    aStrings .Add('Adam');
    aStrings .Add('Eva');
    aStrings .Add('Kain');
    aStrings .Add('Abel');
  end;      
end;

Now this is not to say that returning an object instance is a bad thing every time -- that is the sole function of the very useful Factory pattern, for example. But for more "general" functions like this, the above is a "better" way of doing things.

Answer 7

I would write the code that way, for several reasons:

1) By standardizing how memory allocations look, it is easy to inspect the source code to see if something is missing, without having to read all lines. Here, you just see the .Create, and notice that exception handling is good, so you don't have to read the part between try...except.

2) By standardizing how you code memory allocations, you never forget to do it, even when necessary.

3) If you would later change the source code, insert some code, change how .Add works, replace TStringList with something else, etc., then it would not break this code.

4) It means that you don't have to think about whether TStringList.Add() will throw exceptions, or not, relieving your brain for other work that provides more value.

Answer 8

That kind of code is a classic "factory" pattern. That example is trivial, and may not require exception handling because it could raise exceptions in extreme corner cases. But if the factory code is much more complex, it is correct to free the instance created before re-raising any encountered exception because the caller can't know if the instance was created or not before the exception was raised. It is far better to assume the instance was not created or freed if any exception is returned. Factories are useful when the returned instance may not be always the same (i.e. any class in a given hierarchy), and when parameters to create the instance are "unknonwn" to the caller but not the factory. In such cases the caller can't pass an already created instance just to be "filled" with the proper parameters, but has just to require an instance - of course it has to be clear the caller becomes the "owner" of the created instance.