doing the same thing but with less code

[Anonymous Monk]

Hi all, I inhereted some code that I'd like to clean up. Any way
in doing this with fewer lines.
Thanks,
Mark

        my @short_org_name = keys %myorgs;
        my $count=2;
        my $frame;
        foreach (@short_org_name)
        {
           my $score = $hits{$myorgs{$title_order{$count}}};

           if ($score eq "NoHit")
           {
              $frame = $tab->Label(-bg => 'black', -relief => 'sunken'
+, width => 15);
              $balloon->attach($frame,-balloonmsg => "No Hits");
                         }
           else
           {
              if ($score == 0)
              {
                 $frame = $tab->Label(-bg => 'red', -relief => 'sunken
+', width => 15);
                 $balloon->attach($frame,-balloonmsg => "E = 0");
                 
              }
              if (1e-99 <= $score && $score < 1e-90)
              {
                 $frame = $tab->Label(-bg => 'orange', -relief => 'sun
+ken', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-99 < E < 1
+e-90");
                 
              }
              if (1e-90 <= $score && $score < 1e-80)
              {
                 $frame = $tab->Label(-bg => 'gold', -relief => 'sunke
+n', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-90 < E < 1
+e-80");
                 
              }
              if (1e-80 <= $score && $score < 1e-70)
              {
                 $frame = $tab->Label(-bg => 'yellow', -relief => 'sun
+ken', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-80 < E < 1
+e-70");
                 
              }
              if (1e-70 <= $score && $score < 1e-60)
              {
                 $frame = $tab->Label(-bg => 'chartreuse', -relief => 
+'sunken', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-70 < E < 1
+e-60");
                 
              }
              if (1e-60 <= $score && $score < 1e-50)
              {
                 $frame = $tab->Label(-bg => 'green', -relief => 'sunk
+en', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-60 < E < 1
+e-50");
                 
              }
              if (1e-50 <= $score && $score < 1e-40)
              {
                 $frame = $tab->Label(-bg => 'turquoise', -relief => '
+sunken', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-50 < E < 1
+e-40");
                 
              }
              if (1e-40 <= $score && $score < 1e-30)
              {
                 $frame = $tab->Label(-bg => 'blue', -relief => 'sunke
+n', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-40 < E < 1
+e-30");
                 
              }
              if (1e-30 <= $score && $score < 1e-20)
              {
                 $frame = $tab->Label(-bg => 'pink', -relief => 'sunke
+n', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-30 < E < 1
+e-20");
                 
              }
              if (1e-20 <= $score && $score < 1e-10)
              {
                 $frame = $tab->Label(-bg => 'purple', -relief => 'sun
+ken', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-20 < E < 1
+e-10");
                
              }
              if (1e-10 <= $score && $score < 1)
              {
                 $frame = $tab->Label(-bg => 'grey', -relief => 'sunke
+n', width => 15);
                 $balloon->attach($frame,-balloonmsg => "1e-10 < E < 1
+");
                 
              }
           }

20030817 Edit by Corion: Fixed formatting

[blokhead]

The bulk of all this code repetition seems to be concentrated on determining the size of the number on a logarithmic scale... Instead of brute-force checking a bunch of ranges, you can simply calculate the exponent (1e-XX part) by taking its log base ten. (If you're not aware with the log-identity I've used to get Perl's base-e log function to compute log base 10, consult perldoc -f log)

I take the 10's digit of the exponent as the index of the array of colors, and then code the special cases.. I'll leave it to you to incorporate this into your current code.

my @colors = qw/grey purple pink blue turquoise green
                chartreuse yellow gold orange/;
                                                                      
+          
for my $score (<DATA>) {
  chomp $score;
  my ($color, $label);

  if ( $score > 0 ) {
    my $power      = int( - log( $score ) / log 10 );
    my $tens_place = int( $power / 10 );
    $color         = $colors[ $tens_place ];
    $label         = sprintf( "1e-%d0 < E < 1e-%d0", $tens_place+1,
                                                     $tens_place );
  } elsif ( $score eq 'NoHit' ) {
    $color         = 'black';
    $label         = 'No Hits';

  } else {
    $color         = 'red';
    $label         = 'E = 0';
  }
                                                                      
+          
  print "$score ==> $color ==> $label\n";
}

__DATA__
1e-10
1e-33
1e-93
NoHit
1e-54
1e-3
0
1e-99

output:

1e-10 ==> purple ==> 1e-20 < E < 1e-10
1e-33 ==> blue ==> 1e-40 < E < 1e-30
1e-93 ==> orange ==> 1e-100 < E < 1e-90
NoHit ==> black ==> No Hits
1e-54 ==> green ==> 1e-60 < E < 1e-50
1e-3 ==> grey ==> 1e-10 < E < 1e-00
0 ==> red ==> E = 0
1e-99 ==> orange ==> 1e-100 < E < 1e-90

This seems to work for my very light test cases. I noticed something odd about your existing code: You have a big foreach loop, but never use the value you're looping over ($_). Maybe this is just a copy-n-paste error, but I thought I'd point it out just in case.

blokhead

[Anonymous Monk]

Thanks to everyone for their replies. I shaved the code down considerably. -Mark

[larsen]

You could drop a lot of code starting from this example:

use strict;
use warnings;

use Test::More qw/no_plan/;

my @interval = (
    [  0,         '' ],
    [  5,   'orange' ],
    [ 10,     'grey' ],
    [ 15,     'blue' ],
);

sub interval {
    my $x = shift;

    # The special case 0 should be threated separately
    my $counter = 1;
    while( $counter <= $#interval && $x > $interval[ $counter ][ 0 ] )
+ {
        ++$counter;
    }
    return $interval[ $counter ][ 1 ];
}

ok( interval( 3 )  eq 'orange', "0 < 3 <= 5" ); 
ok( interval( 7 )  eq 'grey',   "5 < 7 <= 10" ); 
ok( interval( 10 ) eq 'grey',   "5 < 10 <= 10" );

Update: Oops. Wrong order of clauses in the while condition. Now fixed.

[antirice]

Determining whether or not code can be shortened requires you to look at what is repeated and what differs between each repetition. Then you can simply write a subroutine to which you pass these differences or you can collect the information you need within some sort of data structure and iterate over it. In this particular case, you could actually just use POSIX's log10 function on $score to get its power of 10 and from there just figure out the powers above and below it. Please note that in the case where log10($score) < -99 that you should probably just say E = 0 (since log10(0) = -Inf). If you get stuck along the way, feel free to post code and ask for help in troublshooting.

Hope this helps.

antirice    
The first rule of Perl club is - use Perl
The ith rule of Perl club is - follow rule i - 1 for i > 1

[bart]

It looks to me like the code all does the same thing: creating a frame with a different color and a different balloon message, on some conditions.

I can see basically two options for refactoring:

1. Put the common code in a sub, and call the sub for every condition
2. Create a loop with the data that differs between loops, as a function of the loop variable.
I'll produce some code showing both options.

The first option: create a sub and call it.

my $attach_frame = sub {
    my($color, $label) = @_;
    my $frame = $tab->Label(-bg => $color, -relief => 'sunken', width 
+=> 15);
    $balloon->attach($frame,-balloonmsg => $label);
}
if ($score eq "NoHit") {
    $attach_frame->('black', 'No Hits');
} else if($score == 0) {
    $attach_frame->('red', 'E = 0');
} else if($score < 1e-99) {
    #eh... nothing?
} else if($score < 1e-90) {
    $attach_frame->('orange', '"1e-99 < E < 1e-90"');
} else if($score < 1e-80) {
    $attach_frame->('gold', '1e-90 < E < 1e-80');
} else if($score < 1e-70) {
    $attach_frame->('yellow', '1e-80 < E < 1e-70');
} else if($score < 1e-60) {
    $attach_frame->('chartreuse', '1e-70 < E < 1e-60');
} else if($score < 1e-50) {
    $attach_frame->('green', '1e-60 < E < 1e-50');
} else if($score < 1e-40) {
    $attach_frame->('turquoise', '1e-50 < E < 1e-40');
} else if($score < 1e-30) {
    $attach_frame->('blue', '1e-40 < E < 1e-30');
} else if($score < 1e-20) {
    $attach_frame->('pink', '1e-30 < E < 1e-20');
} else if($score < 1e-10) {
    $attach_frame->('purple', '1e-20 < E < 1e-10');
} else if($score < 1) {
    $attach_frame->('grey', '1e-10 < E < 1');
}

The second option: create a test loop.

foreach(
  [black  => "NoHit", 'No Hits'],
  [red    => [0]],
  [orange => ['1e-99', '1e-90']],
  [gold   => ['1e-90', '1e-80']],
  [yellow => ['1e-80', '1e-70']],
  [chartreuse => ['1e-70', '1e-60']],
  [green  => ['1e-60', '1e-50']],
  [turquoise => ['1e-50', '1e-40']],
  [blue   => ['1e-40', '1e-30']],
  [pink   => ['1e-30', '1e-20']],
  [purple => ['1e-20', '1e-10']],
  [grey   => ['1e-10', '1']]) {
    my($color, $range, $label) = @$_;
    my $match;
    unless(ref $range) {
        if($match = $score eq $range) {
            defined $label or $label = $range;
        }
    } elsif(@$range == 1) {
        if($match = $score == $range->[0]) {
            defined $label or $label = "E = $range->[0]";
        }
    } else {
        if($match = $score >= $range->[0] && $score < $range->[1]) {
            defined $label or $label = "$range->[0] < E < $range->[1]"
+;
        }
    }
    if($match) {
        $frame = $tab->Label(-bg => $color, -relief => 'sunken', width
+ => 15);
        $balloon->attach($frame,-balloonmsg => $label);
        last;
    }
}