CODEWITHSUNDEEP
perl
aschultz
aschultz

Reputation: 1678

How can I use map to clean up this Perl code?

The code below does what I want it to. It prints the list and adds an asterisk at the end of lines that are not sequential, e.g. if you skip from 1 to 3 or 3 to 5.

use strict;
use warnings;
#note: thanks to all who helped with formatting issues.

#note: I recognize a hash would be a much better option for what I want to do.
my @printy = ("1 -> this",
  "5 -> that",
  "3 -> the other",
  "6 -> thus and such");
@printy = sort {num($a) <=> num($b)} @printy;

my $thisID = 0;
my $lastID = 0; 

#print out (line)* if initial number is >1 more than previous, or just (line) otherwise
for (@printy)
{ 
$thisID = $_; $thisID =~s/ .*//g;
if ($thisID - $lastID != 1) { $_ =~ s/$/ \*/; }
$lastID = $thisID;
}
print join("\n", @printy) . "\n";

sub num
{
  my $x = $_[0];
  $x =~ s/ .*//;
  return $x;
}

But I think I can do better. It feels tangled, and my intuition tells me I'm missing something powerful that could do the job more easily, one that takes maybe two lines.

Now I've used the map() command before, but only to look at/modify an element, not how it compares to a previous element. Can anyone recommend a way to make this more succinct? Thanks!

Upvotes: 2

Views: 179

Answers (6)

zdim
zdim

Reputation: 66901

I'd also advise to clean up the code and keep the loop. However, here is a map based way.

The code uses your sorted @printy and the num sub.

my @nums = map { num($_) } @printy;

my @res = map { 
    $nums[$_] == $nums[$_-1] + 1              # invariably false for $_ == 0
        ? $printy[$_] : $printy[$_] .= ' *'; 
}
(0..$#printy);

say for @res;

This works for the first element since it does not come after the last, given that we're sorted. That may be a bit diabolical though and it needs a comment in code. So perhaps better spell it out

my @res = map { 
    ($nums[$_] == $nums[$_-1] + 1) ? $printy[$_] : $printy[$_] .= ' *'; 
}
(1..$#printy);
unshift @res, $printy[0];

Not as clean but clear.

All this does extra work when compared to a straight loop, of course.

Upvotes: 2

Zaid
Zaid

Reputation: 37146

Since Perl promotes TIMTOWTDI, map may seem like an attractive option at first. Let's see how it fares for this task:

Schwartzian thought process

  1. Since access to neighboring elements is necessary, it's convenient to work with the indices. Since for n elements, there are n-1 pairs of neighbors, you don't have to loop n times. In this case, let's start with 1 instead of the usual 0:

    1 .. $#printy

  2. One can access neighboring elements by calling the relevant indices inside map.

    map { my $prev = $printy[$_-1]; my $curr = $printy[$_] } 1 .. $#printy;

    An array slice expresses this more succinctly:

    map { my ( $prev, $curr ) = @printy[$_-1,$_]; } 1 .. $#printy;

  3. Let's introduce the real logic related to comparing numbers using the num subroutine:

    map {
       my ( $prev, $curr ) = @printy[$_-1,$_];
       if ( num($curr) - num($prev) > 1 ) {
          "$curr *";
       }
       else {
          $curr;
       }
    } 1 .. $#printy;

    Which is equivalent to:

    map {
       my ( $prev, $curr ) = @printy[$_-1,$_];
       $curr .= " *" if num($curr) - num($prev) > 1;
       $curr
    } 1 .. $#printy;

  4. Remember not to forget the first element:

    @printy = ( $printy[0],
            map {
                   my ( $prev, $curr ) = @printy[$_-1,$_];
                   $curr .= " *" if num($curr) - num($prev) > 1;
                   $curr
                } 1 .. $#printy
          );

Given the final result, I'm not so sure I'd use map for this:

  • It's hard to read
  • There's a lot going on
  • The next person working on your code will love you

Upvotes: 7

ikegami
ikegami

Reputation: 385996

Your data yells "Use a hash!" to me.

If we had a hash,

my %printy =
   map { split / -> / }
      "1 -> this", "5 -> that", "3 -> the other", "6 -> thus and such";

The solution would simply be:

my @order = sort { $a <=> $b } keys(%printy); 
for my $i (@order[1..$#order]) {
   $printy{$i} .= ' *'
      if !exists($printy{$i-1});
}

print "$_ -> $printy{$_}\n"
   for @order;

This can be golfed down, though I'm not sure it's worth it.

my $count;
print "$_ -> $printy{$_}".( !$count++ || exists($printy{$_-1}) ? "" : " *" )."\n"
   for
      sort { $a <=> $b }
         keys(%printy);

That for can be converted into a map, but it just makes it less efficient.

my $count;
print
   map { "$_ -> $printy{$_}".( !$count++ || exists($printy{$_-1}) ? "" : " *" )."\n" }
      sort { $a <=> $b }
         keys(%printy);

Upvotes: 3

Borodin
Borodin

Reputation: 126742

I'm sorry, but your code is a shambles, and you need to do much more than use map to clean up this code

You have no indentation and multiple statements on a single line, and you haven't thought through your logic. Your code is unmaintainable

Here's how I would write this. It builds a parallel array of IDs, and then sorts a list of indices so that both the IDs and the original data are in order

If it makes you happier, it does include map

use strict;
use warnings 'all';

my @data = ( '1 -> this', '5 -> that', '3 -> the other', '6 -> thus and such' );

my @ids = map { /(\d+)/ } @data;

my @indexes = sort { $ids[$a] <=> $ids[$b] } 0 .. $#ids;

my $last_id;

for my $i ( @indexes ) {

    print $data[$i];
    print ' *' if defined $last_id and $ids[$i] > $last_id + 1;
    print "\n";

    $last_id = $ids[$i];
}

output

1 -> this
3 -> the other *
5 -> that *
6 -> thus and such

Upvotes: 0

choroba
choroba

Reputation: 241938

No map needed, just add some spaces here and there, and remove stuff that's not needed ($_, join, etc.). Also, reuse num() inside the loop, no need to repeat the regex.

#!/usr/bin/perl
use warnings;
use strict;
use feature qw{ say };

my @printy = sort { num($a) <=> num($b) }
                  '1 -> this', '5 -> that', '3 -> the other', '6 -> thus and such';
my $thisID = my $lastID = 0;

for (@printy) {
    $thisID = num($_);
    $_ .= ' *' if $thisID - $lastID != 1;
    $lastID = $thisID;
}
say for @printy;

sub num {
    my ($x) = @_;
    $x =~ s/ .*//;
    return $x;
}

Also, reimplementing num using /(\d+)/ instead of substitution might tell its purpose more clearly.

Upvotes: 4

simbabque
simbabque

Reputation: 54333

I agree with choroba that there is no need for a map here. But I'd refactor a little bit anyway.

use strict;
use warnings;
use feature 'say';

my @printy = ( "1 -> this", "5 -> that", "3 -> the other", "6 -> thus and such" );

my $last_id = 0;
foreach my $line ( sort { num($a) <=> num($b) } @printy ) {
    my $current_id = num($line);
    $line .= ' *' unless $current_id - $last_id == 1;
    $last_id = $current_id;
}
say for @printy;

# returns the number at the start of a string
sub num {
    $_[0] =~ m/^(\d+)/;
    return $1;
}

  • I moved the sort down into the foreach, because you shouldn't rely on the fact that your input is sorted.

  • I changed the variable names to go with the convention that there should be no capital letters in variable names, and I used say, which is like print with a system-specific newline at the end.

  • I also moved the $current_id into the loop. That doesn't need to be visible outside because it's lexical to that loop. Always declare variables in the smallest possible scope.

  • You already had that nice num function, but you're not using it inside of the loop to get the $current_id. Use it.

I think if the input gets very long, it might make sense to go with a map construct because sorting will be very expensive at some point. Look at the Schwartzian transform for caching the calculation before sorting. You could then do everything at once. But it won't be readable for a beginner any more.

Upvotes: 3

Related Questions