How would you rewrite this?

jacques has asked for the wisdom of the Perl Monks concerning the following question:

Replies are listed 'Best First'.
Re: How would you rewrite this? by davidrw (Prior) on Apr 27, 2005 at 01:28 UTC
Maybe something like (as others have said, assuming no copy/paste errors): `my @checks = ( [ qw/0 agency_id agency_id / ], [ qw/1 advertiser advertiser / ], [ qw/2 compaign_id compaign_id / ], [ qw/3 contact_id admin_contact_id / ], [ qw/3 contact_id tech_contact_id / ], ); foreach my $check (@checks){ my( $idx, $key, $k ) = @$check; get_updates($objects[$idx], $key, $result_ref->{$k}) if (defined $result_ref->{$k} && $result_ref->{$k} != 0); }` [download] Update: Changed "$result_ref->{'agency_id'} != 0" to "$result_ref->{$k} != 0" to reflect OP's correction.	[reply] [d/l]
Re^2: How would you rewrite this? by dragonchild (Archbishop) on Apr 27, 2005 at 12:40 UTC
You can improve that. `my @checks = ( [ qw/0 agency_id agency_id / ], [ qw/1 advertiser advertiser / ], [ qw/2 compaign_id compaign_id / ], [ qw/3 contact_id admin_contact_id / ], [ qw/3 contact_id tech_contact_id / ], ); foreach my $check (@checks){ my( $idx, $key, $k ) = @$check; (defined $result_ref->{$k} && $result_ref->{$k} != 0) && get_updates($objects[$idx], $key, $result_ref->{$k}); }` [download] Note that means that $result_ref->{$k} is allowed to be the empty string. If it's not, then it's better written as a simple check for boolean truth. `$result_ref->{$k} && get_updates($objects[$idx], $key, $result_ref->{$k});` [download] In Perl6, that could look like: `my @checks = qw( 0 agency_id agency_id 1 advertiser advertiser 2 compaign_id compaign_id 3 contact_id admin_contact_id 3 contact_id tech_contact_id ); for (@checks) -> ($idx, $key, $check) { $result_ref->{$check} && get_updates( @objects[$idx], $key, $result_ref->{$check} ); }` [download] The Perfect is the Enemy of the Good.	[reply] [d/l] [select]
Re^2: How would you rewrite this? by jacques (Priest) on Apr 27, 2005 at 01:52 UTC
Change that last hash key to $k and it looks good to me! Thanks.	[reply]
Re: How would you rewrite this? by tlm (Prior) on Apr 27, 2005 at 01:15 UTC
Do you want to test `$result_ref->{'agency_id'}` is non-zero for all all the calls? That looks to me like a typo, since you first test for the "defined-ness" of other unrelated values. If it is not a typo, you could simplify the appearance of your code by putting everyting inside an if-statement conditional on that test. if ( defined $result_ref->{agency_id} and $result_ref->{agency_id} ) +{ get_updates($objects[0], 'agency_id', $result_ref->{'agency_id'}); get_updates($objects[1], 'advertiser', $result_ref->{'advertiser'}) if defined $result_ref->{'advertiser'}; get_updates($objects[2], 'campaign_id', $result_ref->{'campaign_id'} +) if defined $result_ref->{'campaign_id'}; get_updates($objects[3], 'contact_id', $result_ref->{'admin_contact_ +id'}) if defined $result_ref->{'admin_contact_id'}; get_updates($objects[3], 'contact_id', $result_ref->{'tech_contact_i +d'}) if defined $result_ref->{'tech_contact_id'}; ); [download] the lowliest monk	[reply] [d/l]
Re^2: How would you rewrite this? by jacques (Priest) on Apr 27, 2005 at 01:30 UTC
You discovered the typo. Yes, agency_id should not be in every if statement. That was a copy/paste error when I was writing the code. So rewriting it the way you did would not be right.	[reply]
Re: How would you rewrite this? by graff (Chancellor) on Apr 27, 2005 at 01:20 UTC
I was uncertain whether the 5th call should have had `objects[4]` -- in fact, maybe there are "copy/paste" problems in both the 4th and 5th calls. If the calls were supposed to follow a regular pattern (as opposed to the last two calls breaking the pattern), then this would be how I would phrase it: `if ( $result_ref{'agency_id'} != 0 ) { my $i = 0; for my $k (qw/agency_id advertiser campaign_id admin_contact_id te +ch_contact_id) { get_updates($objects[$i++], $k, $result_ref->{$k}) if (defined $result_ref->{$k}); } }` [download] On the other hand, if the last two calls are supposed to be exactly as in the OP (and do not follow a regular a pattern), then make an extra array or hash to provide the necessary values for the index into @objects and for the second arg to get_updates().	[reply] [d/l] [select]
Re^2: How would you rewrite this? by ihb (Deacon) on Apr 27, 2005 at 01:37 UTC
Note that your code doesn't follow the original with regard to the index. You only increase the index if the `if` branch is evaluated. You need to increase `$i` outside the `if`. I'd probably rewrite it to this though: `my @fields = qw/ ... /; for (0 .. $#fields) { get_updates($objects[$_], $fields[$_], $result_ref->{$fields[$_]} if defined $result_ref->{$fields[$_]}; }` [download] A structure like that doesn't invite to get things out of sync. `ihb` See perltoc if you don't know which perldoc to read!	[reply] [d/l] [select]
Re^2: How would you rewrite this? by jacques (Priest) on Apr 27, 2005 at 01:34 UTC
The array indexes are correct. That is not a typo, although I can see how it could look like one. There is only one major typo -- and that was agency_id key in every if statement.	[reply]
Re: How would you rewrite this? by salva (Canon) on Apr 27, 2005 at 09:10 UTC
instead of testing for `... if defined $result_ref->{'agency_id'} && $result_ref +->{'agency_id'} != 0` [download] you can just use `... if $result_ref->{agency_id}` [download] (unless there are agency_id values that are not numbers and you want to skip the if expression on them... but this would be generating warnings with your code, so I suppose there aren't)	[reply] [d/l] [select]
Re: How would you rewrite this? by jonadab (Parson) on Apr 27, 2005 at 12:00 UTC
I could be mistaken, but doesn't `&&` bind more tightly than `!=`? Shouldn't you be putting parens around the second test in each case? If it were me, with such lengthy conditions, I would probably turn the statement modifiers around into regular conditionals, like this... `if (defined $result_ref->{'agency_id'} and $result_ref->{'agency_id'} != 0) { get_updates($objects[0], 'agency_id', $result_ref->{'agency_id'}); }` [download] But that's perhaps just a matter of personal style. "In adjectives, with the addition of inflectional endings, a changeable long vowel (Qamets or Tsere) in an open, propretonic syllable will reduce to Vocal Shewa. This type of change occurs when the open, pretonic syllable of the masculine singular adjective becomes propretonic with the addition of inflectional endings." — Pratico & Van Pelt, BBHG, p68	[reply] [d/l] [select]
Re: How would you rewrite this? by larryk (Friar) on Apr 27, 2005 at 15:22 UTC
hmm. the fiddle with two 3 indexes mapping to contact_id makes it kinda ugly but if the order is not important... `my %stuff = ( agency_id => 0, advertiser => 1, campaign_id => 2, admin_contact_id => 3, tech_contact_id => 3, ); get_updates( $objects[$stuff{$_}], /(contact_id)$/ ? $1 : $_, # ugh $result_ref->{$_} ) for grep $result_ref->{$_}, keys %stuff;` [download] but personally I would add `return unless $_[2];` (or whatever you shifted `$_[2]` into) to get_updates to remove the requirement of checking `$result_ref->{$_}` every time. larryk perl -le "s,,reverse killer,e,y,rifle,lycra,,print"	[reply] [d/l] [select]
A reply falls below the community's threshold of quality. You may see it by logging in.