Bug 10291 - Misleading Variable names in batchMod.pl
Summary: Misleading Variable names in batchMod.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Colin Campbell
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-20 15:54 UTC by Colin Campbell
Modified: 2014-12-07 20:02 UTC (History)
3 users (show)

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


Attachments
proposed patch (4.38 KB, patch)
2013-05-20 16:03 UTC, Colin Campbell
Details | Diff | Splinter Review
Updated patch (3.10 KB, patch)
2013-06-03 14:22 UTC, Colin Campbell
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10291: Clarify misleading variable name (2.79 KB, patch)
2013-06-04 01:54 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10291: Clarify misleading variable name (2.85 KB, patch)
2013-06-17 15:26 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2013-05-20 15:54:16 UTC
batchMod,pl has two variables with names of the form x_hashref but both are actually references to an array of hashrefs. It passes the contents of one to the templares under the name itemnumbers_hashref but the use of it belies that name.

For the sake of future maintainers lets clean up these names
Comment 1 Colin Campbell 2013-05-20 16:03:57 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-06-03 04:38:00 UTC
It looks pointless to me to have hashrefs with only one key 'itemnumber'. Would you consider changing those arrays to be pure itemnumbers please, while you're at it?
Comment 3 Colin Campbell 2013-06-03 14:07:23 UTC
(In reply to comment #2)
> It looks pointless to me to have hashrefs with only one key 'itemnumber'.
> Would you consider changing those arrays to be pure itemnumbers please,
> while you're at it?

Yes in fact that simplifies the patch. Will post an update
Comment 4 Colin Campbell 2013-06-03 14:22:03 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2013-06-04 01:54:45 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2013-06-17 15:26:04 UTC
QA comment:

Variable renaming.
Need to modify the max amount of items (l. 113) and see the html code source for testing this patch.
No regression, no change detected.

Marked as Passed QA.
Comment 7 Jonathan Druart 2013-06-17 15:26:16 UTC
Created attachment 19076 [details] [review]
Bug 10291: Clarify misleading variable name

Remove the unrequired extra processing and pass
the array of itemnumbers as itemnumbers_array

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 8 Galen Charlton 2013-06-17 16:02:29 UTC
Pushed to master.  Thanks, Colin!