in reply to Regarding the conditional part of eslif (or if) statement

Thanks Again Monks.

I see as usual there is more than one way to tackle any problem including being a bad coder LOLS. I did go for the extra elsif statements in the end as shown in the code below as there was only 4 elements in the array. 

my $y =1;
foreach my $key1 (sort keys %outdate) {
if ($outdate{$key1} eq $tacho) {
  $worksheet1->write_blank($r, $y, $tachosquare);
}
if ($outdate{$key1} eq $mot) {
  $worksheet1->write_blank($r, $y, $motsquare);
}
if ($outdate{$key1} eq $motlec) {
  $worksheet1->write_blank($r, $y, $motlecsquare);
}
foreach (@pmis) {
if($outdate{$key1} eq $_){
$worksheet1->write_blank($r, $y, $pmisquare);
 }
}
 if ($lec) {
 if ($lec eq $outdate{$key1} && $tacho eq $outdate{$key1} ) {
  $worksheet1->write($r, $y, "L", $tacholecsquare);
}
 elsif ($pmis[0] eq $outdate{$key1} && $lec eq $outdate{$key1} ){
   $worksheet1->write($r, $y, "L", $pmilecsquare);
 }
   elsif ($pmis[1] eq $outdate{$key1} && $lec eq $outdate{$key1} ){
   $worksheet1->write($r, $y, "L", $pmilecsquare);
 }
  elsif ($pmis[2] eq $outdate{$key1} && $lec eq $outdate{$key1} ){
   $worksheet1->write($r, $y, "L", $pmilecsquare);
 }
   elsif ($pmis[3] eq $outdate{$key1} && $lec eq $outdate{$key1} ){
   $worksheet1->write($r, $y, "L", $pmilecsquare);
 }
else {
 if ($outdate{$key1} eq $lec) {
  $worksheet1->write($r, $y, "L", $lecsquare);
}
}

 }

     $y++;

}

[download]

I could not at the time get my head around a more logical way of writing it

note this code is inside a bigger while loop

Replies are listed 'Best First'.
Re^2: Regarding the conditional part of eslif (or if) statement
by wfsp (Abbot) on Sep 19, 2010 at 09:10 UTC
    Just looking at your 
    if ($lec){
  #...
}

    [download]
    block. As has been suggested replacing all those near indentical elsif blocks with a loop would be better.

    If you have a default, set it at the begining and avoid an else block.

    For within a small scope it also worth using a temp var to hold the value of a var with a long name that will be used many times. This also uses a temp "holding" var ($cell) so that the write method only appears once.

    This approach helps eliminate a lot of repitition which can be confusing and hard to maintain. And it cuts down typing. :-) 

    # give ourselves a short name
# in a short scope
my $o = $outdate{$key1};

if ($lec and $lec = $o) {
  
  # set the default
  my $cell = $lecsquare;
  
  if ($tacho eq $o) {
    $cell = $tacholecsquare;
  }
  else {
    for my $i (0..3){
      $cell = $pmilecsquare if $pmis[$i] eq $o;
    }
  }
  $worksheet1->write($r, $y, "L", $cell);
}

    [download]
[reply]
[d/l]
[select]
      Thanks wfsp you showed me the logic I was missing at the time.

      I also thank you for reminding me to cut down on typing.

[reply]
          for my $i (0..3){
      $cell = $pmilecsquare if $pmis[$i] eq $o;
    }

      [download]
      should be the following to be equivalent: 
          for my $i (0..3){
      if ($pmis[$i] eq $o) {
        $cell = $pmilecsquare;
        last;
      }
    }

      [download]
      which can be simplified to 
          for (@pmis){
      if ($_ eq $o) {
        $cell = $pmilecsquare;
        last;
      }
    }

      [download]
[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom