From 61ba41dffb1bb1037b9fcbbc4b352732c1902e50 Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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 | 2 +- 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 +++-- 15 files changed, 40 insertions(+), 27 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index add0f74..79a42f4 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -434,7 +434,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 7941a70..904552b 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -209,7 +209,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); @@ -226,7 +226,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 fb7aa5d..dec13ed 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -267,17 +267,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 @@ -352,7 +357,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..d374fa0 100755 --- a/acqui/parcel.pl +++ b/acqui/parcel.pl @@ -174,7 +174,7 @@ for my $order ( @orders ) { $line{holds} = 0; my @itemnumbers = GetItemnumbersFromOrder( $order->{ordernumber} ); for my $itemnumber ( @itemnumbers ) { - my ( $count ) = &GetReservesFromBiblionumber($line{biblionumber}, undef, $itemnumber); + my ( $count ) = &GetReservesFromBiblionumber({ biblionumber => $line{biblionumber}, itemnumber => $itemnumber }); $line{holds} += $count; } $line{budget} = GetBudgetByOrderNumber( $line{ordernumber} ); 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 8bdf280..2580f8e 100755 --- a/catalogue/detail.pl +++ b/catalogue/detail.pl @@ -406,7 +406,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 a925285..bd7664d 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 41de4fd..187cf50 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 4e539c4..a89d80c 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--; -- 1.7.10.4