[Koha-patches] [PATCH 2/2] Bug 5415 Let calls of SimpleSearch utilize considtent interface

[LAURENT Henri-Damien]

Le 18/11/2010 17:37, Clay Fouts a écrit :
> The die() in the above code is perfectly legitimate because the routine
> does not know how to handle the error gracefully. I cannot count the
> number of hours I've had to spend tracking a bug back to its source
> through all sorts of twists and turns because Koha did not die() when it
> couldn't figure out how to handle the original error and simply warn()ed
> about it or, more frequently, opted not to check for errors at all.
> 
> The good thing about exceptions is that you cannot ignore the error and
> get away with it. Warn()s can and do get ignored, much to the detriment
> of developers, users, and everyone in between. Every best practices
> guide will tell you to die() as soon as you figure out that you can't
> accommodate the nature of the error, precisely because it is ugly and
> cannot be ignored, forcing the developer to handle it somehow at a
> higher level. Programmers are lazy. Unless we compel ourselves to
> address errors, it's far too easy to ignore them, so we will (and do, if
> Koha's code is to serve as an example). Exceptions demand better coding
> practicing.
ok. But
> 
> Koha already has a CGI-friendly die() handler with CGI::Carp. If how it
> is presented to the user is a concern, the template for that can be
> prettified. Nevertheless, it's a favor to everyone not to let errors
> propagate throughout the system. It corrupts data and gives the false
> perception that everything is behaving correctly.
> 
> Cheers,
> Clay
Hi Clay.
So If in a result list or in a bulkedition, there is one biblio which
can't be decoded (because XML::Parser fails), you are in favor of
returning Error 500 to the user ?
I am not. You may think it hides problems. But if for one record which
has a problem, you throw away all the results by breaking at the first
error, then you end up with having to make kind of a hell of a work only
to find where the error might be and even worse, make the user feel very
uncomfortable with your tool... (Just look at the way zebra is dying
when you have an error with the indexing process. I just think it is not
a sane way to do things.) Yes raise an error, Yes warn that error AND
provide information about the context (biblionumber if it is about
biblios). But No, donot simply die... because when running a
bulkmarcimport on a 100000 records, having to do things over and over
again for 10 records with an error is just insane. Or Please, please,
die in functions, AND add tests to whatever script or function which is
using this other function and try to provide correct information.

> Exceptions demand better coding
> practicing.
I fully agree though. Should we begin some kind of New module for that ?
Croak could be good, but we would have to make things more failsafe and
not fail first error of an array of task.


Friendly.
-- 
Henri-Damien LAURENT