[Koha-patches] [Bug 5379] Pull Request

[Galen Charlton]

Hi,

On Thu, Nov 11, 2010 at 11:08 AM, Chris Nighswonger
<cnighswonger at foundations.edu> wrote:
> I disagree since an undef return is a de facto indicator of success at
> present, doing a return only on a db error and then throwing an error to the
> interface based on that value results in an inconsistency with some of the
> consumers reading the undef return as success and others as failure. Grep
> seems to think that no present consumer looks at the return value of this
> sub, so setting it to something to differentiate an error would not appear
> to hurt any present calls to it and would give a positive indication of an
> error.

Then there's no current reason not to do just a plain 'return;' rather
than add a magic value. :) More on that below.

Of course, there is a broader discussion to be have about how to
handle errors.  Throwing exceptions would be option for us to
consider.

>> [2] Bailing out of AddMember should be done before invoking
[snip]
> I disagree, however, with an undef return.

An undefined value is a clear indication that something went wrong; 0
in this context would be just another magic value.  Magic values are
to be avoided, even if the magic value in question is 0.

Consider

$borrowernum = AddMember(...);

If the warnings stricture is turned on, you're much more likely to get
warnings about using an undefined $borrowernum.  Database updates that
try to use an undefined $borrowernum value will try to look up a
patron by a null ID or try to insert it and will fail safely, whereas
0 as a $borrowernum is not inherently wrong -- it's unlikely that a
patron would have a borrowernumber of 0 in a real Koha database, but
there's nothing that actually enforces that.

Regards,

Galen
-- 
Galen Charlton
gmcharlt at gmail.com