[Koha-devel] Proposed coding standard: always check for object existence before use

[Barton Chittenden]

On Wed, Jun 13, 2018 at 3:07 PM, Jonathan Druart <
jonathan.druart at bugs.koha-community.org> wrote:

> Hi Barton,
>
> Yes, and no. It depends.
>

... the dark side of TMTOWTDI ;-)

I read through your examples, they all made good sense.

Basically, we should avoid indentation blocks (if ( defined $item ) { #
> then continue }), it is usually a bad idea.
>

I'm not quite clear on what you mean here... do you mean that we should
either find a good object or fail gracefully?

I'm going to give a more concrete example... this was new code in 17.11,
but it's no longer in master...

sub GetMemberAccountRecords {
    my ($borrowernumber) = @_;
    my $dbh = C4::Context->dbh;
    my @acctlines;
    my $numlines = 0;
    my $strsth      = qq(
                        SELECT *
                        FROM accountlines
                        WHERE borrowernumber=?);
    $strsth.=" ORDER BY accountlines_id desc";
    my $sth= $dbh->prepare( $strsth );
    $sth->execute( $borrowernumber );

    my $total = 0;
    while ( my $data = $sth->fetchrow_hashref ) {
        if ( $data->{itemnumber} ) {
            my $item = Koha::Items->find( $data->{itemnumber} );
            my $biblio = $item->biblio;

Here, we're traversing accountlines...  $data->{itemnumber} comes from
accountlines. We find the item object based on that itemnumber, and then
attempt to create a biblio record from that... which is fine, as long as
the item hasn't been deleted.

 my $biblio = $item->biblio;

is a fatal error in this case. I don't think that we're doing anything
terribly consequential with $biblio...

I totally agree that how we handle this depends on context... sometimes the
instantiated object is critical, sometimes it's not...  but I would assert
that we *always* need to check to make sure that objects are defined before
they're used.

   my $object = Koha::Foo->find( $something );
   $object->{bar};

or

   my $object = Koha::Foo->new();
   $object->{bar};

Without any error checking on $object should be a violation of coding
standards... assuming that $object is defined simply isn't safe.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.koha-community.org/pipermail/koha-devel/attachments/20180613/44af7052/attachment.html>