From 962a7057d043773e1cf23dfde0195cadde941a86 Mon Sep 17 00:00:00 2001 From: Colin Campbell Date: Fri, 26 Aug 2011 16:17:11 +0100 Subject: [PATCH] Bug 6790: Remove unnecessary variable from getroutinglist return getroutinglist returns a count variable to indicate how many elements are in the array. This is almost always a serious code smell. (We are programming in a list manipulating language) The routine was executing am unnecessary loop just to maintain that var. Removed the variable from the routine and perldoc refactored calls of the routine removed the c-style loops for more idiomatic and maintainable for loops renamed some opaquely named variables removed a call to the routine where nothing was done with the data moved some html out of the calling script and into the template Signed-off-by: Chris Cormack --- C4/Serials.pm | 17 ++---- .../prog/en/modules/serials/routing.tt | 28 +++++++-- serials/routing-preview.pl | 39 +++++------- serials/routing.pl | 64 ++++++------------- serials/subscription-detail.pl | 2 +- 5 files changed, 64 insertions(+), 86 deletions(-) diff --git a/C4/Serials.pm b/C4/Serials.pm index 1471184..39f2389 100644 --- a/C4/Serials.pm +++ b/C4/Serials.pm @@ -2027,12 +2027,11 @@ sub delroutingmember { =head2 getroutinglist -($count,@routinglist) = getroutinglist($subscriptionid) +@routinglist = getroutinglist($subscriptionid) this gets the info from the subscriptionroutinglist for $subscriptionid return : -a count of the number of members on routinglist the routinglist as an array. Each element of the array contains a hash_ref containing routingid - a unique id, borrowernumber, ranking, and biblionumber of subscription @@ -2042,20 +2041,14 @@ sub getroutinglist { my ($subscriptionid) = @_; my $dbh = C4::Context->dbh; my $sth = $dbh->prepare( - "SELECT routingid, borrowernumber, ranking, biblionumber + 'SELECT routingid, borrowernumber, ranking, biblionumber FROM subscription JOIN subscriptionroutinglist ON subscription.subscriptionid = subscriptionroutinglist.subscriptionid - WHERE subscription.subscriptionid = ? ORDER BY ranking ASC - " + WHERE subscription.subscriptionid = ? ORDER BY ranking ASC' ); $sth->execute($subscriptionid); - my @routinglist; - my $count = 0; - while ( my $line = $sth->fetchrow_hashref ) { - $count++; - push( @routinglist, $line ); - } - return ( $count, @routinglist ); + my $routinglist = $sth->fetchall_arrayref({}); + return @{$routinglist}; } =head2 countissuesfrom diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt index 85c42b1..9851ebf 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt @@ -47,16 +47,30 @@ function search_member(subscriptionid){ [% END %] [% issue %] -[% IF ( memberloop ) %] +[% IF memberloop %]
  • Recipients: - -[% FOREACH memberloo IN memberloop %] - - - -[% END %] + + + [% USE m_loop = iterator(memberloop) %] + [% FOREACH member IN m_loop %] + + + + + [% END %]
    Name RankDelete
    [% memberloo.name %][% memberloo.routingbox %]Delete
    Delete
    [% member.name %] + + Delete

    Add recipients   Delete All

    • diff --git a/serials/routing-preview.pl b/serials/routing-preview.pl index 64849b5..6746f53 100755 --- a/serials/routing-preview.pl +++ b/serials/routing-preview.pl @@ -56,7 +56,7 @@ if($edit){ print $query->redirect("routing.pl?subscriptionid=$subscriptionid"); } -my ($routing, @routinglist) = getroutinglist($subscriptionid); +my @routinglist = getroutinglist($subscriptionid); my $subs = GetSubscription($subscriptionid); my ($tmp ,@serials) = GetSerials($subscriptionid); my ($template, $loggedinuser, $cookie); @@ -81,18 +81,17 @@ if($ok){ my $const = 'o'; my $notes; my $title = $subs->{'bibliotitle'}; - for(my $i=0;$i<$routing;$i++){ - my $sth = $dbh->prepare("SELECT * FROM reserves WHERE biblionumber = ? AND borrowernumber = ?"); - $sth->execute($biblio,$routinglist[$i]->{'borrowernumber'}); - my $data = $sth->fetchrow_hashref; + for my $routing ( @routinglist ) { + my $sth = $dbh->prepare('SELECT * FROM reserves WHERE biblionumber = ? AND borrowernumber = ? LIMIT 1'); + $sth->execute($biblio,$routing->{borrowernumber}); + my $reserve = $sth->fetchrow_hashref; - # warn "$routinglist[$i]->{'borrowernumber'} is the same as $data->{'borrowernumber'}"; - if($routinglist[$i]->{'borrowernumber'} == $data->{'borrowernumber'}){ - ModReserve($routinglist[$i]->{'ranking'},$biblio,$routinglist[$i]->{'borrowernumber'},$branch); - } else { - AddReserve($branch,$routinglist[$i]->{'borrowernumber'},$biblio,$const,\@bibitems,$routinglist[$i]->{'ranking'},'',$notes,$title); - } - } + if($routing->{borrowernumber} == $reserve->{borrowernumber}){ + ModReserve($routing->{ranking},$biblio,$routing->{borrowernumber},$branch); + } else { + AddReserve($branch,$routing->{borrowernumber},$biblio,$const,\@bibitems,$routing->{ranking}, undef, undef, $notes,$title); + } + } } ($template, $loggedinuser, $cookie) @@ -115,15 +114,11 @@ if($ok){ }); } -my @results; -my $data; -for(my $i=0;$i<$routing;$i++){ - $data=GetMember('borrowernumber' => $routinglist[$i]->{'borrowernumber'}); - $data->{'location'}=$data->{'branchcode'}; - $data->{'name'}="$data->{'firstname'} $data->{'surname'}"; - $data->{'routingid'}=$routinglist[$i]->{'routingid'}; - $data->{'subscriptionid'}=$subscriptionid; - push(@results, $data); +my $memberloop = []; +for my $routing (@routinglist) { + my $member = GetMember( borrowernumber => $routing->{borrowernumber} ); + $member->{name} = "$member->{firstname} $member->{surname}"; + push @{$memberloop}, $member; } my $routingnotes = $serials[0]->{'routingnotes'}; @@ -134,7 +129,7 @@ $template->param( issue => $issue, issue_escaped => URI::Escape::uri_escape($issue), subscriptionid => $subscriptionid, - memberloop => \@results, + memberloop => $memberloop, routingnotes => $routingnotes, hasRouting => check_routing($subscriptionid), ); diff --git a/serials/routing.pl b/serials/routing.pl index 2bd7d69..f093da7 100755 --- a/serials/routing.pl +++ b/serials/routing.pl @@ -60,13 +60,13 @@ if($op eq 'add'){ addroutingmember($borrowernumber,$subscriptionid); } if($op eq 'save'){ - my $sth = $dbh->prepare("UPDATE serial SET routingnotes = ? WHERE subscriptionid = ?"); + my $sth = $dbh->prepare('UPDATE serial SET routingnotes = ? WHERE subscriptionid = ?'); $sth->execute($notes,$subscriptionid); my $urldate = URI::Escape::uri_escape($date_selected); print $query->redirect("routing-preview.pl?subscriptionid=$subscriptionid&issue=$urldate"); } -my ($routing, @routinglist) = getroutinglist($subscriptionid); +my @routinglist = getroutinglist($subscriptionid); my $subs = GetSubscription($subscriptionid); my ($count,@serials) = GetSerials($subscriptionid); my $serialdates = GetLatestSerials($subscriptionid,$count); @@ -86,65 +86,41 @@ foreach my $dateseq (@{$serialdates}) { } my ($template, $loggedinuser, $cookie) -= get_template_and_user({template_name => "serials/routing.tmpl", += get_template_and_user({template_name => 'serials/routing.tmpl', query => $query, - type => "intranet", + type => 'intranet', authnotrequired => 0, flagsrequired => {serials => 'routing'}, debug => 1, }); -my @results; -my $data; -for(my $i=0;$i<$routing;$i++){ - $data=GetMember('borrowernumber' => $routinglist[$i]->{'borrowernumber'}); - $data->{'location'}=$data->{'branchcode'}; - if ($data->{firstname} ) { - $data->{name} = $data->{firstname} . q| |; +my $member_loop = []; +for my $routing ( @routinglist ) { + my $member=GetMember('borrowernumber' => $routing->{borrowernumber}); + $member->{location} = $member->{branchcode}; + if ($member->{firstname} ) { + $member->{name} = $member->{firstname} . q| |; } else { - $data->{name} = q{}; + $member->{name} = q{}; } - if ($data->{surname} ) { - $data->{name} .= $data->{surname}; + if ($member->{surname} ) { + $member->{name} .= $member->{surname}; } - $data->{'routingid'}=$routinglist[$i]->{'routingid'}; - $data->{'subscriptionid'}=$subscriptionid; - if (! $routinglist[$i]->{routingid} ) { - $routinglist[$i]->{routingid} = q||; - } - my $rankingbox = '"; - $data->{'routingbox'} = $rankingbox; - - push(@results, $data); -} + $member->{routingid}=$routing->{routingid} || q{}; + $member->{ranking} = $routing->{ranking} || q{}; -# for adding routing list -my $new; -if ($op eq 'new') { - $new = 1; -} else { -# for modify routing list default - $new = 0; + push(@{$member_loop}, $member); } $template->param( - title => $subs->{'bibliotitle'}, + title => $subs->{bibliotitle}, subscriptionid => $subscriptionid, - memberloop => \@results, - op => $new, + memberloop => $member_loop, + op => $op eq 'new', dates => $dates, routingnotes => $serials[0]->{'routingnotes'}, hasRouting => check_routing($subscriptionid), ); - output_html_with_http_headers $query, $cookie, $template->output; +output_html_with_http_headers $query, $cookie, $template->output; diff --git a/serials/subscription-detail.pl b/serials/subscription-detail.pl index 7386820..8e7f418 100755 --- a/serials/subscription-detail.pl +++ b/serials/subscription-detail.pl @@ -46,7 +46,7 @@ if ($op && $op eq 'del') { print "Content-Type: text/html\n\n"; exit; } -my ($routing, @routinglist) = getroutinglist($subscriptionid); + my ($totalissues,@serialslist) = GetSerials($subscriptionid); $totalissues-- if $totalissues; # the -1 is to have 0 if this is a new subscription (only 1 issue) # the subscription must be deletable if there is NO issues for a reason or another (should not happend, but...) -- 1.7.5.4