What pitfalls does this Perl code have?

Question

I have written some code to print formatted arrays (first line = no of inputs, second line = max width of numbers). The star can be any sort of marker to differentiate some elements from the rest.

$ cat inp.txt
6
2
1 *
2
3
4
9
12 *
$ cat inp.txt | ./formatmyarray.pl
 ____ ____ ____ ____ ____ ____ 
| *  |    |    |    |    | *  |
|  1 |  2 |  3 |  4 |  9 | 12 |
|____|____|____|____|____|____|
$

fomatmyarray.pl

#!/usr/bin/perl
use warnings;
use strict;

my $spc = q{ };
my $und = q{_};
my $sep = q{|};
my $end = "\n";
my @inp = <STDIN>;
my $len = $inp[0];
my $wid = $inp[1];
chomp $wid;

sub printwall {

    my($left, $middle, $l, $w) = @_;

    for(0..($l - 1)) {
        if ($_ == 0) { print $left; }
        for(1..($w + 2)) { print $middle; }
        print $left;
    }
    print $end;
 return;
}

sub printchar {

    my($left, $middle, $l, $w) = @_;

    for(0..($l - 1)) {
        if ($_ == 0) { print $left; }
        my @temp = split ' ', $inp[$_ + 2];

        my $star = 0;
        if (($#temp) >= 1) { $star = 1;    }

        my $mid = sprintf "%d", (($w + 2) /2);
        for(1..($w + 2)) {
            if (($_ == $mid) && ($star == 1)) { print "*"; }
            else { print $middle; }
        }
        print $left;
    }
    print $end;
 return;
}

sub printnum {

    my($left, $middle, $l, $w) = @_;

    for(0..($l - 1)) {
        if ($_ == 0) { print $left; }
        my @temp = split ' ', $inp[$_ + 2];

        my $format = join '', q{%}, $w, q{d};
        my $num = sprintf($format, $temp[0]);

        print join '', $middle, $num, $middle, $left;
    }
    print $end;
 return;
}

printwall($spc, $und, $len, $wid);
printchar($sep, $spc, $len, $wid);
printnum ($sep, $spc, $len, $wid);
printwall($sep, $und, $len, $wid);

I already checked it with Perl::Critic but that will only tell me the syntactical problems (which I have already corrected). Are there any problems that you see with this code. Something an experienced Perl programmer would do differently?

Any comments or suggestions are welcome.

Answer 1

There's a lot of room for improvement here (I'll update this answer as and when I have time).

Let's start off with the inputs. You should not have to specify the number of entries or their maximum length as Perl can infer that for you:

my $entries   = my @entries = <STDIN>;

---

Don't forget about CPAN.

For instance, consider Text::ASCIITable.

Answer 2

• The return statements would not appear in most people's code - a sub returns when it reaches the end (but see discussion in comments).

• In printwall, I'd unconditionally print the first left wall outside the loop; ditto the other functions.

• I'm not convinced I'd read all the data into @inp as shown. More likely, I'd use:

  my $num = <STDIN>;  # Or, more likely, just <>
  my $wid = <STDIN>;
  my @inp = <STDIN>;
  

  This would clean up the $inp[$_ + 2] in the functions.

• I'd probably pass the array to the functions, rather than using global variables - globals are grubby in Perl as everywhere else.

• The count of the number of values is not needed in the input. With the array containing just the data to be printed, you can iterate over each member of the array in the functions with a suitable foreach, improving its Perlishness.

• In printnum, you can build the format string once (not each iteration).

• This:

  my $mid = sprintf "%d", (($w + 2) /2);
  

  is a funny way of writing:

  my $mid = int(($w + 2) / 2);
  

• I'd probably use a regex to find the star; it isn't clear whether you should print a '*' if any character is found, or if you should print the character that is found.

• I'd probably be using a single format to deal with the stars:

  my $fmt = sprintf "%*s%%c%*s%c", $wid, $middle, $wid, $middle, $left;
  

  I might need to tune one of the $wid values to allow for even widths, but the output would be:

  " %c  |"
  

  You can then simply print each cell with a blank or a '*' for the value using the format.

• Similarly, in printnum, I'd be generating a simple format string like " %2d |" to print each number - and I'd generate that format once.

Etc.

Answer 3

Several suggestions in here. Hope this is helpful.

use strict;
use warnings;
use Getopt::Long qw(GetOptions);

my $SPC = q{ };
my $UND = q{_};
my $SEP = q{|};
my $END = "\n";

main();

sub main {
    # Try to keep run options and core input data separate from each other.
    GetOptions('max=i' => \my $max_n);

    # Parse input at the outset so that subsequent methods
    # don't have to worry about such low-level details.
    my $inp = parse_input();

    # Prune the input array at the outset.
    # This helps to keep subsequent methods simpler.
    splice @$inp, $max_n if $max_n;

    # Don't require the user to compute max width.
    my $wid = determine_width($inp);

    # The format string can be defined at the outset.
    my $fmt = join '', $SEP, $SPC, '%', $wid, 's', $SPC;

    # You can print both data and stars using one method.
    print_border($inp, $wid, $SPC);
    print_content($inp, $fmt, $_) for qw(star data);
    print_border($inp, $wid, $SEP);
}

sub parse_input {
    my @parsed;
    # Using <> provides more flexibility than <STDIN>.
    while (<>){
        chomp;
        my ($value, $star) = split;
        $star = $SPC unless defined $star;
        push @parsed, { data => $value, star => $star }
    }
    return \@parsed;
}

sub determine_width {
    my $inp = shift;
    my $wid = 0;
    for (@$inp){
        my $len = length $_->{data};
        $wid = $len if $len > $wid;
    }
    return $wid;
}

# Because we did work at the outset to create a data structure
# that represents our goals conveniently, generating output
# is much simpler.

sub print_border {
    my ($inp, $wid, $wall_sep) = @_;
    print $wall_sep, $UND x ($wid + 2) for @$inp;
    print $wall_sep, $END;
}

sub print_content {
    my ($inp, $fmt, $mode) = @_;
    printf $fmt, $_->{$mode} for @$inp;
    print $SEP, $END;
}