|
Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:
Hi Monks!
I have the same kind of logic in my code, it seems redundant and I am trying to improve it, but I am running out of options. Is there a better way to have all this code in a hash and match against what comes in from the
my $item = $q->param( 'item' ) || '';
Here is the code I am trying to improve:
#!/usr/bin/perl
use strict;
use warnings;
use CGI;
use Data::Dumper;
my $q = CGI->new();
print $q->header();
my $item = $q->param( 'item' ) || '';
my $change;
# Get values
if($item=~/test/) {
$change = "blue";
}elsif($item=~/dark/){
$change = "black";
}elsif($item=~/white/){
$change = "light";
}elsif($item=~/house/){
$change = "home";
}elsif($item=~/all things/){
$change = "multi";
}elsif($item=~/money/){
$change = "value";
}elsif($item=~/country/){
$change = "USA";
}else{
$change = "neutral";
}
print "Final Item = $change";
Thanks for the help! |
Re: Improving if statements
by Your Mother (Archbishop) on Sep 18, 2014 at 23:20 UTC
|
my %stuff = ( moo => "cow",
test => "blue",
dark => "black",
white => "light",
house => "home",
"all things" => "multi",
money => "value",
country => "Well, not Scotland apparently" );
my $whoopdie = $stuff{param("item")} || "NAOAOAOANOOOO!!!";
print "Final Item = $whoopdie";
| [reply]
[d/l] |
|
|
Hash, unlike if, has no order. If the order matters, you might need something like the following (alternatives in regexes match from the leftmost one):
my @keys
= ('test', 'dark', 'white', 'house', 'all things', 'money', 'count
+ry');
my @values
= qw(blue black light home multi value USA);
my $regex = join '|', @keys; # "map quotemeta" might be needed for ugl
+ier keys
$regex = qr/($regex)/;
my %switch;
@switch{@keys} = @values;
my $change;
if ($item =~ $regex) {
$change = $switch{$1};
} else {
$change = "neutral";
}
print "Final Item = $change.\n";
| [reply]
[d/l] |
|
|
Order will not matter, I am more concerned about speed.
| [reply] |
|
|
I liked it!
I just fixed this line for future reference:
From this:
my $whoopdie = $stuff{param("item")} || "NAOAOAOANOOOO!!!";
To:
my $whoopdie = $stuff{$q->param("item")} || "NAOAOAOANOOOO!!!";
Thanks for the helping! | [reply]
[d/l]
[select] |
|
|
| [reply]
[d/l] |
Re: Improving if statements
by Bloodnok (Vicar) on Sep 19, 2014 at 08:13 UTC
|
TIMTOWTDI, if order matters, as it tends to in a test (.t file), I frequently use something along the lines of ...
# Defaine default
my $change = "neutral";
my @lookup = (
{ test => "blue" },
{ dark => "black" },
{ white => "light" },
{ house => "home" },
{ all\ things => "multi" },
{ money => "value" },
{ country => "USA" },
);
while (my ($i, $c) = each %lookup) {
next unless $item eq $i;
$change = $c;
last;
}
A purely untested suggestion ...
A user level that continues to overstate my experience :-))
| [reply]
[d/l] |
Re: Improving if statements
by biohisham (Priest) on Sep 19, 2014 at 05:15 UTC
|
Multiple if statements are really redundant, that is true, but not necessarily a bad thing unless readability of the code becomes difficult. I find the solutions by Your Mother and choroba to be very educational, succinct and efficient. It may be also relevant to include the utility of the Switch module for tasks like testing different cases and responsing to each case.
use strict;
use warnings;
use Switch;
my $item = $ARGV[0];
switch($item){
case "moo" {print "cow"}
case "test" {print "blue"}
case "dark" {print "black"}
case "white" {print "light"}
case "house" {print "home"}
case "all things" {print "multi"}
case "money" {print "value"}
case "country" {print "Well, not Scotland apparently"}
else {print "NAOAOAOANOOOO!!!" }
}
UPDATE: The use of switch statements in the way I recommended may not be a favorable option judging by the amount of downvotes my reply received. Thanks Tobylink for elaborating on problems with Switch statements, I definitely learned something new about them.
David R. Gergen said "We know that second terms have historically been marred by hubris and by scandal." and I am a two y.o. monk today :D, June,12th, 2011...
| [reply]
[d/l] |
|
|
Don't recommend Switch please. The abstract in its documentation says (my emphasis):
A switch statement for Perl, do not use if you can use given/when
It continues:
There are undoubtedly serious bugs lurking somewhere in code this funky :-) Bug reports and other feedback are most welcome.
May create syntax errors in other parts of code.
On perl 5.10.x may cause syntax error if "case" is present inside heredoc.
In general, use given/when instead. It were introduced in perl 5.10.0. Perl 5.10.0 was released in 2007.
The given and when keywords were introduced in Perl 5.10. They are experimental, and will warn on some versions of Perl (unless you disable that warning), but still a lot more reliable than Switch.
There's also Switch::Plain and Switcheroo on CPAN.
| [reply]
[d/l]
[select] |
Re: Improving if statements
by sundialsvc4 (Abbot) on Sep 18, 2014 at 23:39 UTC
|
Personally, I don’t find the present way of doing it objectionable. Not at all. It’s obvious what it’s doing. However, you could put the keys into a hash and then loop through it ...
my %stuff = ( moo => "cow",
test => "blue",
dark => "black",
white => "light",
house => "home",
"all things" => "multi",
money => "value",
);
while (my ($k, $v) = each(%stuff)) {
if { $input =~ /$k/ } {
change = $v;
last; # break out of the loop now
}
But is it “improved?” I probably wouldn’t think so, really, because an if..elsif..else cascade is both extremely readable and easy to change, and you can put the most-common cases right up at the top. Basically, unless you have a compelling reason other than vague thoughts of “efficiency” to change the code as it is now, I’d move on to the next to-do ticket.
| [reply]
[d/l] |
|
|
| [reply] |
|
|
Presumably the programmer or others on the team are paying attention to typos, and is writing test cases for every case to prove that the code works and that it continues to do so. Mmmmmm...?
No matter how you did it, there will always be someone else out there who will not only say that you did it “wrong,” but will also insist that the team should spend its time re-writing code that works. (A practice that Warren Buffett refers to as “sucking your thumb.”)
On the one hand, if you truly do have “choose this word if that word appears,” as in this case, some sort of loop-based, “run through this list,” algorithm makes good sense ... and I briefly suggested one. But on the other hand, sometimes the latest request (that cannot be denied) from The Marketing Department throws a monkey-wrench ... an exception to the rule, any slight twist that makes one case a little different from all the rest ... that can make “too-clever” logic turn into a quagmire. And you can’t anticipate (Marketers are from Jupiter ...) what they might come up with someday, when you write the original version. Sometimes, a coding style that is, “frankly, butt-ugly,” wins because it is more maintainable.
To touch on “maintainability” once more: what if the third case, and only the third one, for some reason had to be different from the rest? How much of the existing, tested code would have to be changed? How many of the other, already known-to-be working cases would be impacted? How much of that block of source-code would have to be touched, whether-or-not it had to do “with case number three?” If you were too-clever, the answer would be, “all of them, all of it,” because you wrote the code in such a way that the handling for all of the cases was coupled together. Because the code “cleverly” reduces all of the cases into one, it “cleverly” treats all cases in the same way, but, in so doing, it cannot treat any of them in even a slightly different way.
“Right or wrong?” “Better or worse?” Well, it depends.™ It’s not entirely a matter of “principles.” You do the best you can, and then the men from Jupiter come along to screw it all up ...
| [reply] |
|
|
|
|
| [reply] |
|
|
