Bug 5453

Summary: Variables declaration should not occur in conditionals
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: Architecture, internals, and plumbingAssignee: Galen Charlton <gmcharlt>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: enhancement    
Priority: PATCH-Sent (DO NOT USE) CC: chris, cnighswonger, koha.sekjal, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Proposed Patch
Proposed Patch
Proposed Patch
Proposed Patch
Proposed Patch
Proposed Patch
Proposed Patch
Bug 5453 Do not declare variables in conditionals
Bug 5453: Move declarations out of conditionals
Bug 5453: Move declarations from within conditionals in catalogue/search.pl
Bug 5453 Move declarations out of conditionals
Bug 5453 Move declarations out of conditionals in opac
Bug 5453 Move declarations out of conditionals
Bug 5453 : Move declarations out of conditionals

Description Colin Campbell 2010-11-29 10:13:09 UTC
This is an antipattern in a lot of Koha code.

my $x = Z() if $y;

it obscures code and can cause hard to track bugs. What does it say of the value of $x if not $y? Is someone maintaining the code liable to make unwarrantable assumptions about the definedness or value of $x. ( the use of postfix if is also a problem as in reading actual code it is often easy to miss that an assignment is in a conditional because the postfix if has disappeared off the right of your screen. Postfix ifs give a reader very little visual clue that a conditional is occurring.

The above cde should be rewritten as
my $x;
if ($y) {
    $x = Z();
}

Bug opened to record such patches against
Comment 1 Chris Nighswonger 2010-11-29 20:36:05 UTC
I realize Conway says that postfixed if's are not good style, but I think that as this is a coding "style" issue, it should at least be given an airing on the devel list prior to being implemented.
Comment 2 Colin Campbell 2010-12-02 11:18:54 UTC
The issue isn't the postfix if per se but the declaration of the variable within a the conditional. 
It can cause run time warnings when the undefined undeclined variable is used later in the code
Comment 4 Marcel de Rooy 2011-05-28 12:44:20 UTC
Could you attach the proposed patches?
Comment 5 Colin Campbell 2011-06-02 14:51:56 UTC Comment hidden (obsolete)
Comment 6 Colin Campbell 2011-06-02 14:52:33 UTC Comment hidden (obsolete)
Comment 7 Colin Campbell 2011-06-02 14:52:59 UTC Comment hidden (obsolete)
Comment 8 Colin Campbell 2011-06-02 14:53:40 UTC Comment hidden (obsolete)
Comment 9 Colin Campbell 2011-06-02 14:55:08 UTC Comment hidden (obsolete)
Comment 10 Colin Campbell 2011-06-02 14:55:40 UTC Comment hidden (obsolete)
Comment 11 Colin Campbell 2011-06-02 14:56:23 UTC Comment hidden (obsolete)
Comment 12 Colin Campbell 2011-06-02 15:01:52 UTC
Patches rebased against current HEAD and attached or they can be pulled from the bug_5453 branch on git://github.com/colinsc/koha.git
Comment 13 Chris Cormack 2011-06-14 20:56:28 UTC
Created attachment 4455 [details] [review]
Bug 5453 Do not declare variables in conditionals

Removed occurences in acqui/*.pl

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 14 Chris Cormack 2011-06-14 20:57:46 UTC
Created attachment 4456 [details] [review]
Bug 5453: Move declarations out of conditionals

Patch admin/*.pl scripts

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 15 Chris Cormack 2011-06-14 21:03:41 UTC
Created attachment 4457 [details] [review]
Bug 5453: Move declarations from within conditionals in catalogue/search.pl

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 16 Chris Cormack 2011-06-14 21:05:41 UTC
Created attachment 4458 [details] [review]
Bug 5453 Move declarations out of conditionals

Fix im members scripts

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 17 Chris Cormack 2011-06-14 21:09:31 UTC
Created attachment 4459 [details] [review]
Bug 5453 Move declarations out of conditionals in opac

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 18 Chris Cormack 2011-06-14 21:11:19 UTC
Created attachment 4460 [details] [review]
Bug 5453 Move declarations out of conditionals

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 19 Chris Cormack 2011-06-14 21:14:19 UTC
Created attachment 4461 [details] [review]
Bug 5453 : Move declarations out of conditionals

Patched for C4 Modules

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 20 Chris Cormack 2011-06-14 21:15:48 UTC
Thanks for doing this Colin, it quiets the logs a lot!
Comment 21 Ian Walls 2011-07-14 22:29:34 UTC
1st attachment: acqui/*.pl:  Passed QA

2nd attachment:  admin/*.pl:  Passed QA

3rd attachment: catalogue/search.pl:  Passed QA

4th attachment: members/*.pl: Passed QA

5th attachment: opac/*.pl:  Passed QA

6th attachment: lots of others:  Passed QA

7th attachment: will require more time to read through, as the changes seem more complex, and will affect more of the system.  Please stand by.
Comment 22 Ian Walls 2011-07-15 02:49:05 UTC
7th attachment: C4 modules:  Passed QA.  Marking bug report as such.
Comment 23 Chris Cormack 2011-07-15 03:50:50 UTC
Pushed, please test
Comment 24 Jared Camins-Esakov 2012-12-31 00:27:24 UTC
There have been no further reports of problems so I am marking this bug resolved.