Bug 6790 - C4::Serials::getroutinglist returns unnecessary variable
Summary: C4::Serials::getroutinglist returns unnecessary variable
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Colin Campbell
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-26 15:16 UTC by Colin Campbell
Modified: 2013-12-05 20:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (12.06 KB, patch)
2011-08-26 15:57 UTC, Colin Campbell
Details | Diff | Splinter Review
Modified Patch (12.20 KB, patch)
2011-08-30 13:44 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 6790: Remove unnecessary variable from getroutinglist return (12.22 KB, patch)
2011-12-31 07:27 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2011-08-26 15:16:13 UTC
The subroutine getroutinglist returns a "count" variable along with the array. Not only is this unnecessary but it causes the routine to execute an unnecessary
loop and results in some ugly, unidiomatic code in the calling cgi. Refactor count away.
Comment 1 Colin Campbell 2011-08-26 15:57:21 UTC Comment hidden (obsolete)
Comment 2 Sébastien Marie 2011-08-27 10:03:19 UTC
Your patch use an unsupported form of template style, that may cause problem in translation processing.

In 'serials/routing.tt', you wrote:
<option[%- IF r == member.ranking -%] selected="selected"[% END %] value="[% r %]">[% r %]</option>

The "IF" directive is *in* tag 'option': this kind of construction is unsupported.
(but not problem when directive are in attribute, like 'r' with the 'value' attribute).

Your should use instead of:
[% IF r == member.ranking %]
<option selected="selected" value="[% r %]">[% r %]</option>
[% ELSE %]
<option value="[% r %]">[% r %]</option>
[% END %]

For more detail about this problem, please see bug 6458.
Comment 3 Colin Campbell 2011-08-30 13:44:53 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-10-24 11:38:16 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 5 Paul Poulain 2011-10-25 15:05:51 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 6 Chris Cormack 2011-12-31 07:27:48 UTC
Created attachment 6990 [details] [review]
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 <chris@bigballofwax.co.nz>
Comment 7 Ian Walls 2012-01-21 16:22:51 UTC
Removes for loops in favour of foreach.  Does some minor cleanup to quotes and spacing to clean various coding style violations.  Looks solid, marking as Passed QA
Comment 8 Paul Poulain 2012-01-23 17:06:01 UTC
QA comment: t/db_dependent/lib/KohaTest/Serials.pm does not work after this patch, but it was not working before the patch, so it's OK.

patch pushed
Comment 9 Jared Camins-Esakov 2012-12-31 00:32:24 UTC
There have been no further reports of problems so I am marking this bug resolved.