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
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.
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
This bug is mentioned in: Bug 5453 Do not declare variables in conditionals http://lists.koha-community.org/pipermail/koha-patches/2010-November/013116.html Bug 5453: Move declarations out of conditionals http://lists.koha-community.org/pipermail/koha-patches/2010-November/013120.html Bug 5453: Move declarations from within conditionals in catalogue/search.pl http://lists.koha-community.org/pipermail/koha-patches/2010-November/013121.html Bug 5453 Move declarations out of conditionals http://lists.koha-community.org/pipermail/koha-patches/2010-November/013122.html Bug 5453 Move declarations out of conditionals in opac http://lists.koha-community.org/pipermail/koha-patches/2010-November/013123.html Bug 5453 Move declarations out of conditionals http://lists.koha-community.org/pipermail/koha-patches/2010-November/013124.html Bug 5453 : Move declarations out of conditionals http://lists.koha-community.org/pipermail/koha-patches/2010-November/013125.html
Could you attach the proposed patches?
Created attachment 4340 [details] [review] Proposed Patch
Created attachment 4341 [details] [review] Proposed Patch
Created attachment 4342 [details] [review] Proposed Patch
Created attachment 4343 [details] [review] Proposed Patch
Created attachment 4344 [details] [review] Proposed Patch
Created attachment 4345 [details] [review] Proposed Patch
Created attachment 4346 [details] [review] Proposed Patch
Patches rebased against current HEAD and attached or they can be pulled from the bug_5453 branch on git://github.com/colinsc/koha.git
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>
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>
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>
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>
Created attachment 4459 [details] [review] Bug 5453 Move declarations out of conditionals in opac Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 4460 [details] [review] Bug 5453 Move declarations out of conditionals Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
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>
Thanks for doing this Colin, it quiets the logs a lot!
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.
7th attachment: C4 modules: Passed QA. Marking bug report as such.
Pushed, please test
There have been no further reports of problems so I am marking this bug resolved.