[Koha-patches] [PATCH 40/78] C4/Budgets.pm

[Joe Atzberger]

There are some problems I see with this code:

   - Always exports 27 functions.  Can't we use EXPORT_OK and EXPORT_TAGS
   here?
   - Doesn't use warnings
   - Does use unconditional warn statement
   - 11 $sth->finish calls
   - uses CGI::scrolling_list and hardcoded English terminology
   - low level HTML string substitution like s/ /\ /g

I'll put some other notes inline.

-- 
Joe Atzberger
LibLime - Open Source Library Solutions

On Thu, May 28, 2009 at 12:32 PM, <paul.poulain at biblibre.com> wrote:

> +sub CheckBudgetParent {
> +    my ( $new_parent, $budget ) = @_;
> +    my $new_parent_id = $new_parent->{'budget_id'};
> +    my $budget_id     = $budget->{'budget_id'};
> +    my $dbh           = C4::Context->dbh;
> +    my $parent_id_tmp = $new_parent_id;
> +
> +    # check new-parent is not a child (or a child's child ;)
> +    my $sth = $dbh->prepare(qq|
> +        SELECT budget_parent_id FROM
> +            aqbudgets where budget_id = ? | );
> +    while (1) {
> +        $sth->execute($parent_id_tmp);
> +        my $res = $sth->fetchrow_hashref;
> +        if ( $res->{'budget_parent_id'} == $budget_id ) {
> +            return 1;
> +        }
> +        if ( not defined $res->{'budget_parent_id'} ) {
> +            return 0;
> +        }
> +        $parent_id_tmp = $res->{'budget_parent_id'};
> +    }
> +}
>


The check for not defined should come first, so that warnings can be
enabled.  The second check should be an elsif.  The two possibilities are
exclusive.  You also must be sure that you don't have circular data, or this
loop goes infinite and crashes.


+sub DelBudgetPeriod() {
> +       my ($budget_period_id) = @_;
> +       my $dbh = C4::Context->dbh;
> +         ; ## $total = number of records linked to the record that must be
> deleted
> +    my $total = 0;


Variable not used in this scope.

+# -------------------------------------------------------------------
> +sub GetBudgetHierarchy {
> ....
> +    warn "Q : $query";


Unconditional warn.


> =head3 GetCurrencies
> +
> + at currencies = &GetCurrencies;
> +
> +Returns the list of all known currencies.
> +
> +C<$currencies> is a array; its elements are references-to-hash, whose
> +keys are the fields from the currency table in the Koha database.


Which is it:  @currencies or $currencies?

+END { }    # module clean-up code here (global destructor)


This isn't OO code: this stub does nothing.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20090616/16675d95/attachment-0002.htm>