2

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!

10
  • 7
    I think some indentation and a proper iteration variable would make this code longer, but way more readable. You don't always have to do the fancy stuff. Sometimes clarity is way better. :)
    – simbabque
    Dec 6, 2016 at 14:43
  • 2
    I agree with @simbabque. Rather than using something that you clearly think of as advanced, you should "clean up your code" by indenting it properly and adding blank lines to delineate associated statements.
    – Borodin
    Dec 6, 2016 at 15:17
  • 2
    @Borodin Based on this Code Review meta post I wouldn't be surprised if they refuse migration. This question belongs on SO as far as they're concerned.
    – Zaid
    Dec 6, 2016 at 16:17
  • 1
    @aschultz: So you removed indentation, newlines and blank lines before you posted your code on Stack Overflow?
    – Borodin
    Dec 6, 2016 at 16:36
  • 2
    @Borodin I'm new to asking questions. And I wanted to bring focus to the part of code I felt was most important. I don't fully know what I'm doing, but I do want to learn to do better.
    – aschultz
    Dec 6, 2016 at 16:40

6 Answers 6

7

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
7
  • 4
    With time I've learnt to treat any map with more than two statements in it as a code smell. Better off to rewrite it as a for loop before new requirements find their way in the code :)
    – Zaid
    Dec 6, 2016 at 15:03
  • 2
    I completely agree. But doing the Schwartzian transform in a loop is also very long. I think it's about finding a compromise between easy to read for beginners and not ugly to read for professionals. It really depends on what it is, but once in a while I'd go with the shorter, more concise version and add an explanatory comment and then hope that the next guy (or future me) will learn something from that. :D
    – simbabque
    Dec 6, 2016 at 15:06
  • 4
    @Borodin this answer isn't about giving the OP a fish, it's about teaching them how to fish. They thought that map would help and I wanted to walk them through why it may not be the magic bullet they think/thought it is/was. The "I'm not so sure..." comment follows through on this train of thought, which is why it appears at the end as a conclusion.
    – Zaid
    Dec 6, 2016 at 16:27
  • 2
    @Zaid I'd be surprised at whoever downvoted your response too. I didn't know about Schwartzian transforms, and this was educational for me, and it's good to know that my original approach didn't need to be fancy S
    – aschultz
    Dec 6, 2016 at 16:29
  • 6
    @Borodin 1) of course it's a genuine attempt at answering the question. Why would it be anything but that? 2) I encourage to to (re)read the OP's comment. They appreciated that the "original approach [needn't] be fancy" 3) Schwartzian transforms can be a great idea if used appropriately
    – Zaid
    Dec 6, 2016 at 16:44
4

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.

0
3

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.

3

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);
2
  • I never thought of a hash. I was processing data I'd already gotten. I use them extensively, but somehow it never struck me to use it here. Thanks for tipping off some other cleanup I can use for my original code.
    – aschultz
    Dec 6, 2016 at 16:32
  • 1
    Added a solution that primarily uses map.
    – ikegami
    Dec 6, 2016 at 17:10
2

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.

6
  • @ikegami Thank you -- it's funny how it works for the first element (and too tricky) ...
    – zdim
    Dec 6, 2016 at 19:25
  • Yeah, was just looking at that. When $_ == 0, $nums[$_] == $nums[$_-1] + 1 is $nums[0] == $nums[-1] + 1, which will invariably be false as desired. Tricky (unobvious) indeed!
    – ikegami
    Dec 6, 2016 at 19:26
  • Right, it's interesting, I added a note. Hardly code to recommend though.
    – zdim
    Dec 6, 2016 at 19:27
  • Nothing a comment (# This will invariably be false for $_ == 0.) can't fix!
    – ikegami
    Dec 6, 2016 at 19:28
  • @ikegami Right! A very reasonable solution since the code itself is cleaner. I'll add that, thank you :)
    – zdim
    Dec 6, 2016 at 19:31
0

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.