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, <[email protected]> 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 > + > +...@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.
_______________________________________________ Koha-patches mailing list [email protected] http://lists.koha.org/mailman/listinfo/koha-patches
