From 342cb7e1c02175bda05f8e924fbab4687b93e0cd Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@biblibre.com>
Date: Mon, 18 Mar 2013 14:37:18 +0000
Subject: [PATCH] Bug 9823: Refactor return from GetReservesFromBiblionumber
The return from GetReservesFromBiblionumber contains an unnecessary
extra variable. In scalar context an array returns its element count.
Maintaing a separate count can lead to unforeseen bugs
and imposes ugly constructions on the subroutine's users.
Remove the useless count variable from the return
This patch also changes the parameters: now the routine takes a hashref.
---
C4/Biblio.pm | 2 +-
C4/ILSDI/Services.pm | 4 ++--
C4/Reserves.pm | 19 ++++++++++++-------
C4/SIP/ILS/Item.pm | 4 ++--
acqui/parcel.pl | 4 ++--
catalogue/ISBDdetail.pl | 3 ++-
catalogue/MARCdetail.pl | 3 ++-
catalogue/detail.pl | 3 ++-
catalogue/imageviewer.pl | 3 ++-
catalogue/labeledMARCdetail.pl | 3 ++-
catalogue/moredetail.pl | 3 ++-
opac/opac-detail.pl | 4 ++--
opac/opac-reserve.pl | 4 ++--
reserve/request.pl | 5 +++--
serials/routing-preview.pl | 5 +++--
t/db_dependent/Holds.t | 10 +++++-----
16 files changed, 46 insertions(+), 33 deletions(-)
diff --git a/C4/Biblio.pm b/C4/Biblio.pm
index dc40d68..330951d 100644
--- a/C4/Biblio.pm
+++ b/C4/Biblio.pm
@@ -443,7 +443,7 @@ sub DelBiblio {
# We delete any existing holds
require C4::Reserves;
- my ($count, $reserves) = C4::Reserves::GetReservesFromBiblionumber($biblionumber);
+ my $reserves = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber });
foreach my $res ( @$reserves ) {
C4::Reserves::CancelReserve({ reserve_id => $res->{'reserve_id'} });
}
diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm
index 9a86d2e..f5e6747 100644
--- a/C4/ILSDI/Services.pm
+++ b/C4/ILSDI/Services.pm
@@ -208,7 +208,7 @@ sub GetRecords {
# Get most of the needed data
my $biblioitemnumber = $biblioitem->{'biblioitemnumber'};
- my @reserves = GetReservesFromBiblionumber( $biblionumber, undef, undef );
+ my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber });
my $issues = GetBiblioIssues($biblionumber);
my $items = GetItemsByBiblioitemnumber($biblioitemnumber);
@@ -225,7 +225,7 @@ sub GetRecords {
# Hashref building...
$biblioitem->{'items'}->{'item'} = $items;
- $biblioitem->{'reserves'}->{'reserve'} = $reserves[1];
+ $biblioitem->{'reserves'}->{'reserve'} = $reserves;
$biblioitem->{'issues'}->{'issue'} = $issues;
push @records, $biblioitem;
diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index f066404..7f3c5fb 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -268,17 +268,22 @@ sub GetReserve {
=head2 GetReservesFromBiblionumber
- ($count, $title_reserves) = GetReservesFromBiblionumber($biblionumber);
+ my $reserves = GetReservesFromBiblionumber({
+ biblionumber => $biblionumber,
+ itemnumber => $itemnumber,
+ all_dates => 1|0
+ });
-This function gets the list of reservations for one C<$biblionumber>, returning a count
-of the reserves and an arrayref pointing to the reserves for C<$biblionumber>.
+This function gets the list of reservations for one C<$biblionumber>,
+returning an arrayref pointing to the reserves for C<$biblionumber>.
=cut
sub GetReservesFromBiblionumber {
- my ($biblionumber) = shift or return (0, []);
- my ($all_dates) = shift;
- my ($itemnumber) = shift;
+ my ( $params ) = @_;
+ my $biblionumber = $params->{biblionumber} or return [];
+ my $itemnumber = $params->{itemnumber};
+ my $all_dates = $params->{all_dates} // 0;
my $dbh = C4::Context->dbh;
# Find the desired items in the reserves
@@ -353,7 +358,7 @@ sub GetReservesFromBiblionumber {
}
push @results, $data;
}
- return ( $#results + 1, \@results );
+ return \@results;
}
=head2 GetReservesFromItemnumber
diff --git a/C4/SIP/ILS/Item.pm b/C4/SIP/ILS/Item.pm
index ba85cd7..af9ff1f 100644
--- a/C4/SIP/ILS/Item.pm
+++ b/C4/SIP/ILS/Item.pm
@@ -95,8 +95,8 @@ sub new {
}
my $borrower = GetMember(borrowernumber=>$issue->{'borrowernumber'});
$item->{patron} = $borrower->{'cardnumber'};
- my ($whatever, $arrayref) = GetReservesFromBiblionumber($item->{biblionumber});
- $item->{hold_queue} = [ sort priority_sort @$arrayref ];
+ my $reserves = GetReservesFromBiblionumber({ biblionumber => $item->{biblionumber} });
+ $item->{hold_queue} = [ sort priority_sort @$reserves ];
$item->{hold_shelf} = [( grep { defined $_->{found} and $_->{found} eq 'W' } @{$item->{hold_queue}} )];
$item->{pending_queue} = [( grep {(! defined $_->{found}) or $_->{found} ne 'W' } @{$item->{hold_queue}} )];
$self = $item;
diff --git a/acqui/parcel.pl b/acqui/parcel.pl
index ad0e51d..3f19490 100755
--- a/acqui/parcel.pl
+++ b/acqui/parcel.pl
@@ -174,8 +174,8 @@ for my $order ( @orders ) {
$line{holds} = 0;
my @itemnumbers = GetItemnumbersFromOrder( $order->{ordernumber} );
for my $itemnumber ( @itemnumbers ) {
- my ( $count ) = &GetReservesFromBiblionumber($line{biblionumber}, undef, $itemnumber);
- $line{holds} += $count;
+ my $holds = GetReservesFromBiblionumber({ biblionumber => $line{biblionumber}, itemnumber => $itemnumber });
+ $line{holds} += scalar( @$holds );
}
$line{budget} = GetBudgetByOrderNumber( $line{ordernumber} );
$totalprice += $order->{unitprice};
diff --git a/catalogue/ISBDdetail.pl b/catalogue/ISBDdetail.pl
index 28b5c2d..dd1c5c8 100755
--- a/catalogue/ISBDdetail.pl
+++ b/catalogue/ISBDdetail.pl
@@ -113,7 +113,8 @@ $template->param (
);
-my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1);
+my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
+my $holdcount = scalar( @$holds );
$template->param( holdcount => $holdcount, holds => $holds );
output_html_with_http_headers $query, $cookie, $template->output;
diff --git a/catalogue/MARCdetail.pl b/catalogue/MARCdetail.pl
index 835568a..73e44be 100755
--- a/catalogue/MARCdetail.pl
+++ b/catalogue/MARCdetail.pl
@@ -336,7 +336,8 @@ $template->param (
searchid => $query->param('searchid'),
);
-my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1);
+my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
+my $holdcount = scalar( @$holds );
$template->param( holdcount => $holdcount, holds => $holds );
output_html_with_http_headers $query, $cookie, $template->output;
diff --git a/catalogue/detail.pl b/catalogue/detail.pl
index a3df03b..7099656 100755
--- a/catalogue/detail.pl
+++ b/catalogue/detail.pl
@@ -418,7 +418,8 @@ if (C4::Context->preference('TagsEnabled') and $tag_quantity = C4::Context->pref
'sort'=>'-weight', limit=>$tag_quantity}));
}
-my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1);
+my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
+my $holdcount = scalar ( @$holds );
$template->param( holdcount => $holdcount, holds => $holds );
my $StaffDetailItemSelection = C4::Context->preference('StaffDetailItemSelection');
if ($StaffDetailItemSelection) {
diff --git a/catalogue/imageviewer.pl b/catalogue/imageviewer.pl
index c96e5af..7f994f6 100755
--- a/catalogue/imageviewer.pl
+++ b/catalogue/imageviewer.pl
@@ -78,7 +78,8 @@ $template->{VARS}->{'norequests'} = $norequests;
$template->param(C4::Search::enabled_staff_search_views);
$template->{VARS}->{'biblio'} = $biblio;
-my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1);
+my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
+my $holdcount = scalar( @$holds );
$template->param( holdcount => $holdcount, holds => $holds );
output_html_with_http_headers $query, $cookie, $template->output;
diff --git a/catalogue/labeledMARCdetail.pl b/catalogue/labeledMARCdetail.pl
index e82e3b9..20a73d0 100755
--- a/catalogue/labeledMARCdetail.pl
+++ b/catalogue/labeledMARCdetail.pl
@@ -137,7 +137,8 @@ $template->param (
searchid => $query->param('searchid'),
);
-my ( $holdcount, $holds ) = C4::Reserves::GetReservesFromBiblionumber($biblionumber,1);
+my $holds= C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
+my $holdcount = scalar( @$holds );
$template->param( holdcount => $holdcount, holds => $holds );
output_html_with_http_headers $query, $cookie, $template->output;
diff --git a/catalogue/moredetail.pl b/catalogue/moredetail.pl
index 91877cc..8b07f15 100755
--- a/catalogue/moredetail.pl
+++ b/catalogue/moredetail.pl
@@ -217,7 +217,8 @@ $template->param(ONLY_ONE => 1) if ( $itemnumber && $showncount != @items );
$template->{'VARS'}->{'searchid'} = $query->param('searchid');
-my ( $holdcount, $holds ) = GetReservesFromBiblionumber($biblionumber,1);
+my $holds = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
+my $holdcount = scalar( @$holds );
$template->param( holdcount => $holdcount, holds => $holds );
output_html_with_http_headers $query, $cookie, $template->output;
diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl
index 77de12f..e083075 100755
--- a/opac/opac-detail.pl
+++ b/opac/opac-detail.pl
@@ -539,8 +539,8 @@ for ( C4::Context->preference("OPACShowHoldQueueDetails") ) {
}
my $has_hold;
if ( $show_holds_count || $show_priority) {
- my ($reserve_count,$reserves) = GetReservesFromBiblionumber($biblionumber);
- $template->param( holds_count => $reserve_count ) if $show_holds_count;
+ my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber });
+ $template->param( holds_count => scalar( @$reserves ) ) if $show_holds_count;
foreach (@$reserves) {
$item_reserves{ $_->{itemnumber} }++ if $_->{itemnumber};
if ($show_priority && $_->{borrowernumber} == $borrowernumber) {
diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index df55c19..6d5d46f 100755
--- a/opac/opac-reserve.pl
+++ b/opac/opac-reserve.pl
@@ -147,8 +147,8 @@ foreach my $biblioNumber (@biblionumbers) {
}
# Compute the priority rank.
- my ( $rank, $reserves ) =
- GetReservesFromBiblionumber( $biblioNumber, 1 );
+ my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblioNumber, all_dates => 1 });
+ my $rank = scalar( @$reserves );
$biblioData->{reservecount} = 1; # new reserve
foreach my $res (@{$reserves}) {
my $found = $res->{found};
diff --git a/reserve/request.pl b/reserve/request.pl
index e915dbf..1b303c7 100755
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -196,7 +196,8 @@ foreach my $biblionumber (@biblionumbers) {
}
# get existing reserves .....
- my ( $count, $reserves ) = GetReservesFromBiblionumber($biblionumber,1);
+ my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
+ my $count = scalar( @$reserves );
my $totalcount = $count;
my $holds_count = 0;
my $alreadyreserved = 0;
@@ -446,7 +447,7 @@ foreach my $biblionumber (@biblionumbers) {
# existingreserves building
my @reserveloop;
- ( $count, $reserves ) = GetReservesFromBiblionumber($biblionumber,1);
+ $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
foreach my $res ( sort {
my $a_found = $a->{found} || '';
my $b_found = $a->{found} || '';
diff --git a/serials/routing-preview.pl b/serials/routing-preview.pl
index 154d477..1e0bd09 100755
--- a/serials/routing-preview.pl
+++ b/serials/routing-preview.pl
@@ -71,8 +71,9 @@ if($ok){
if (C4::Context->preference('RoutingListAddReserves')){
# get existing reserves .....
- my ($count,$reserves) = GetReservesFromBiblionumber($biblio);
- my $totalcount = $count;
+ my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblio });
+ my $count = scalar( @$reserves );
+ my $totalcount = $count;
foreach my $res (@$reserves) {
if ($res->{'found'} eq 'W') {
$count--;
diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t
index 2c28c07..76373e4 100755
--- a/t/db_dependent/Holds.t
+++ b/t/db_dependent/Holds.t
@@ -71,8 +71,8 @@ foreach my $borrowernumber ( @borrowernumbers ) {
}
-my ($count, $reserves) = GetReservesFromBiblionumber($biblionumber);
-is( $count, $borrowers_count, "Test GetReserves()" );
+my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber });
+is( scalar(@$reserves), $borrowers_count, "Test GetReserves()" );
my ( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber);
@@ -87,8 +87,8 @@ ok( GetReserveCount( $borrowernumbers[0] ), "Test GetReserveCount()" );
CancelReserve({ 'reserve_id' => $reserve_id });
-($count, $reserves) = GetReservesFromBiblionumber($biblionumber);
-ok( $count == $borrowers_count - 1, "Test CancelReserve()" );
+$reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber });
+is( scalar(@$reserves), $borrowers_count - 1, "Test CancelReserve()" );
( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber);
@@ -148,7 +148,7 @@ my $reserve2 = GetReserveInfo( $reserve->{'reserve_id'} );
ok( $reserve->{'reserve_id'} eq $reserve2->{'reserve_id'}, "Test GetReserveInfo()" );
-($count, $reserves) = GetReservesFromBiblionumber($biblionumber,1);
+$reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
$reserve = $reserves->[1];
AlterPriority( 'top', $reserve->{'reserve_id'} );
$reserve = GetReserve( $reserve->{'reserve_id'} );
--
1.7.10.4