Error handling considerations

[chronozphere]

Hi everyone.

I was looking into the asphyre source. And i must admit, that it's an impressive piece of work. The code looks very clean and it all fits together in a very elegant way.

One thing i noticed was that there wasn't much error handling code present.
Only error checking like this:

[pascal]
if not Succeeded( ARiskyAPICall) then
exit;
[/pascal]

Okay... This is the way i do it:

[pascal]
Result := ARiskyAPICall;
if not Succeeded(Result) then
begin
ReportError( Result, 'A Risky API Call failed' , CurrentModule);
Exit;
end;
[/pascal]

Report error assembles the information and adds it to the log-file.

My approach results in bloated routines. It takes 6 lines instead of 2 and therefore it is much work to handle every error in the entire engine and give them all nice descriptive error-strings. The Asphyre way looks much better and cleaner.

So i started thinking.... Do i need all this error-handling code :? ?

When i throw away my error-handling, I will get much cleaner code and i dont have to think about the errorstrings. So generally: maintaining your code will be easier but debugging it might be harder.

I like to know your experiences with this issue.

What is your approach?
When do you handle/log your errors and when not?
Do you find it easy to debug code with Asphyre-Style error checking?

Thanx a bunch!

[technomage]

Personally, I think error handling is important. If another develper is using your tools/libary and it doesn't do what it's expecting yuo should really tell them by way of results (being false) or via raising exceptions.

As for a user, resonalble error handling is good, a normal user getting a highly technical error message is just going to get confused.

so I guess the answer depends on who you are writing the code for..

[LP]

There are several ways to handle errors. One way is to throw an exception, access violation or reboot the system. Another way is to determine the best course of action when the error occurs so the system continues to function.

This is IMHO an example of bad VCL design. I've seen in several occasions applications throwing "Index out of bounds" error (they forgot to disable exceptions in retail build), which shouldn't really occur.

If you open "Vectors2px.pas" (a unit, which provides TPoint2px record - the same as TPoint, but with operator overloads and a list of TPoint2px entries), check "TPoints2px.GetItem" routine:

function TPoints2px.GetItem(Num: Integer): PPoint2px;
begin
 if &#40;Num >= 0&#41;and&#40;Num < DataCount&#41; then Result&#58;= @Data&#91;Num&#93;
  else Result&#58;= nil;
end;

The above code checks whether the index is inside the range and returns the appropriate value, instead of just throwing the error, so the end application can determine what to do with it.

In some places Asphyre uses "silent error handling", which is essential in Direct3D app, instead of throwing exception and locking the application in full-screen mode, so user has to open Task Manager to close the app.

In some places, simple error checking is made by returning boolean. While developing Direct3D app, you should use Debug Runtimes to get extended error information. However, there is no need to show these detailed reports to your end users, just tell them to update DirectX to 9.0c, install latest video drivers and make sure they have hardware accelerated video card.

[{MSX}]

I think IMHO that the way of handling error of getItem (returning nil) is not good
First, it doesn't solve the problem of having to check every function returning something:

[pascal]a := getItem()
if a==nil then
begin
log();
exit;
end;
b := getItem()
if b==nil then
begin
log();
exit;
end;
c := getItem()
if c==nil then
begin
log();
exit;
end;
[/pascal]

Second, returning nil will likely throw an access violation (nil referencing) if you forget to check the result before using it. The AV can sometimes be triky to track down. And if you don't forget to check the result, maybe your fellow coders will do

I think the right way to do error checking is with Exceptions. They were indeed invented for this reason. I learned to love them in Java

For example, you can do this:

[pascal]
try
a := getItem();
b := getItem();
c := getItem();
except
log();
end;
[/pascal]

That looks way better. Even more, you don't need a try except if the caller have its own since the exceptions propagate to the first except block in the stack.

[pascal]
procedure wasteTime();
begin
a := getItem();
b := getItem();
c := getItem();
end;
[/pascal]

This mean that you can just get rid of 90% of the "if error then log exit" stuff. I've seen by experience that a couple of try/except well placed around can make all the job.
Using diffenent Exception subclasses helps a lot: for example one that you catch inside a loop, and one more severe that propagate to the main.

The problem of the "if error then log and exit" is that it grows with every call. And call inside call needs them too, and so on. That way a program almost grows of three times.
With exceptions, you don't need a check for each call, and not even one for each function.

The indexOutOfBound exception is not that bad IMHO. You can use exceptions just as IFs:

[pascal]
a=accessArray(i);
if a=nil then
begin
log;
exit;
end
else
begin
doStuff(a);
end;
[/pascal]
[pascal]
try
a:=accessArray(i);
doStuff(a);
except IndexOutOfBound
log();
end;
[/pascal]
I do this only if i handle all necessary stuff in the except block. If i have to report the fact to the caller, i let the exception propagate instead.

[LP]

MSX, your argument is equally flawed at exceptions. In retail product, if you disable exceptions, then you risk of getting access violations. You can't predict all possible executions of your software. Also, if your user forgets to catch an exception, it is equal to crashing (the application behavior may be unpredicted after that). [size=7px]Actually, IMHO, if you would raise exception in list for index being out of bounds, I consider it as a improperly handled input, although it's a matter of opinion.[/size]

There is no point of start arguing whether the particular behavior of GetItem is good or bad (in YOUR opinion), in either case it is a VALID behavior. It handles all cases properly. Whether the user checks for nil or not is up to the user.

My point was that your application should handle as much input and behave correctly as possible, instead of just throwing error messages.

For instance: you set ScrollBar.Max to 150, then ScrollBar.Position to 100, then ScrollBar.Max to 50, the application should continue working instead of throwing error "Position is beyond max limit!".

[cairnswm]

Personally I think giving a default result is even better than raising any sort of error in a game. A game you want to be using as many clock cycles as possible for the game play and not coding error handling on every event.

So for a list, if an index is selected that doesn't exist return the first/last item instead.

This allows the developer just to develop their code and never worry about impossible results.

[{MSX}]

MSX, your argument is equally flawed at exceptions. In retail product, if you disable exceptions, then you risk of getting access violations.

Uhm i don't disable exceptions on retail producs.. They don't waste so much resources as to disable them, i think.

Also, if your user forgets to catch an exception, it is equal to crashing (the application behavior may be unpredicted after that).

It's not true that the behaviour may be unpredicted (well, it may of course, but for other reasons), becouse an exception is always catched by some handler. If you forget one, the more external one with take it. An exception can't be forgotten, by definition. Eventually with no handler, the whole application will crash, which is better IMHO than continuing with wrong behaviour.

More important, you get an immediate error (a more talking one than a generic AV error btw). With AV, the error can (and very often does) pop up in a completely different place:
[pascal]
// here i set myItem property of an object:
myObject.myItem := getItem(x); // this raises exception instantly, with the offending line number. No need to debug
...
// another unit, another thread, another caller far far away:
yourObject.myItem.getValue(); // access violation. But who set myItem to nil? Need to debug
[/pascal]

There is no point of start arguing whether the particular behavior of GetItem is good or bad (in YOUR opinion), in either case it is a VALID behavior. It handles all cases properly. Whether the user checks for nil or not is up to the user.

But i stated that all was IMHO
They have a valid behaviour in the sense that they both work, but as i showed up here, they're quite different. I think the exception is better looking and better serving.

My point was that your application should handle as much input and behave correctly as possible, instead of just throwing error messages.

I think differently: i think an application should just take what can chew. If an application can't make sense of what it reads, using it could cause strange behaviour.

For instance: you set ScrollBar.Max to 150, then ScrollBar.Position to 100, then ScrollBar.Max to 50, the application should continue working instead of throwing error "Position is beyond max limit!".

In this example i agree with you, but in this case the application can make sense of the inexpected situation, it can eventually just scale the position.
Most depend also on the contract of each method or class. If it can be elastic without being wrong, i'm all with it.

But in many case there is something that you can't make sense of.
I agree with cairnswm that returning a default value (or doing the default behaviour) can be a good choiche, but you don't always have a default value.

I apologize if i look picky or worst I'm just showing my POV, i didn't intended to be crude.

[arthurprs]

At start of my game i was writing tons of error handling things, later because of the big time/line spending i stoped.

[Mirage]

In debug mode application should report all things that can be considired as errors. A reference to an invalid item in a list most likely means a logical error in your code and you should get to know about it ASAP. If you simply clamp invalid index to a valid range you will not have a chance to find the error. So I would GetItem be as follows:

function TPoints2px.GetItem(Num: Integer): PPoint2px; 
begin 
  Assert&#40;&#40;Num >= 0&#41;and&#40;Num <DataCount>= 0&#41;and&#40;Num < DataCount&#41; then Result&#58;= @Data&#91;Num&#93; 
  else Result&#58;= nil; 
end;

In case of a "risky API call" fail it may be useful to write a line to log even in release version. It will help to find and fix problems remotely by end-user requests.

[Mirage]

Concerning exceptions - they have pros and cons:
+ all error-handling code in one (or more) places
+ an error occured in a low-lvel routine can be handled somewhere at a higher-level

- performance penalty even when no exception raised
- in a low-level library exceptions are inappropriate - some programmers (or projects) use them, some not, and the library should not to force use of exceptions.

So what to do? Fortunately Object Pascal has such a good thing as delegates (method pointers). The idea is to have a method-pointer variable within a library and when an error occurs, call the delegate. As an example I show how error handling is done in my stream class:

TError = class(TMessage)
    ErrorMessage: string;
    constructor Create(AErrorMessage: string);
    class function DefaultErrorHandler(const Error: TError): Boolean;
  end;

  TErrorHandler = function(const Error: TError): Boolean of object;

  EStreamError = class(TError)
  end;
  EInvalidFormat = class(TError)
  end;
  EFileError = class(TError)
  end;
...
var 
  ErrorHandler: TErrorHandler;
...
function TFileStream.Read(out Buffer; const Count: Cardinal): Cardinal;
begin
  Result := 0;
  if not Opened then if not ErrorHandler(EStreamError.Create('File stream is not opened')) then Exit;
  BlockRead(F, Buffer, Count, Result);
  if Result > 0 then FPosition := FPosition + Result;
end;

The default ErrorHandler simply logs an error class and its text message. A library user can override this behaviour and make the delegate to raise exceptions (if he likes to use them) or even make a chain of error handlers at various levels of the application.

This solution doesn't have any performance penalties and has all advantages of exception-based approach.
Additionally, an error handler can fix the problem occured, return True and the routine which called the handler will continue its job. This can not be achieved with exceptions or result-code based approach.

What do you think about it, guys?

P.S.: It seems that a post can not have more than one code section?