Bug 5453 - Variables declaration should not occur in conditionals
Summary: Variables declaration should not occur in conditionals
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: PATCH-Sent (DO NOT USE) enhancement (vote)
Assignee: Galen Charlton
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-29 10:13 UTC by Colin Campbell
Modified: 2013-12-05 20:05 UTC (History)
4 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 (2.74 KB, patch)
2011-06-02 14:51 UTC, Colin Campbell
Details | Diff | Splinter Review
Proposed Patch (5.90 KB, patch)
2011-06-02 14:52 UTC, Colin Campbell
Details | Diff | Splinter Review
Proposed Patch (4.97 KB, patch)
2011-06-02 14:52 UTC, Colin Campbell
Details | Diff | Splinter Review
Proposed Patch (3.91 KB, patch)
2011-06-02 14:53 UTC, Colin Campbell
Details | Diff | Splinter Review
Proposed Patch (11.29 KB, patch)
2011-06-02 14:55 UTC, Colin Campbell
Details | Diff | Splinter Review
Proposed Patch (8.77 KB, patch)
2011-06-02 14:55 UTC, Colin Campbell
Details | Diff | Splinter Review
Proposed Patch (13.98 KB, patch)
2011-06-02 14:56 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 5453 Do not declare variables in conditionals (2.75 KB, patch)
2011-06-14 20:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5453: Move declarations out of conditionals (5.91 KB, patch)
2011-06-14 20:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5453: Move declarations from within conditionals in catalogue/search.pl (4.97 KB, patch)
2011-06-14 21:03 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5453 Move declarations out of conditionals (3.91 KB, patch)
2011-06-14 21:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5453 Move declarations out of conditionals in opac (11.30 KB, patch)
2011-06-14 21:09 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5453 Move declarations out of conditionals (8.77 KB, patch)
2011-06-14 21:11 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5453 : Move declarations out of conditionals (13.99 KB, patch)
2011-06-14 21:14 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 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.