Какие подводные камни есть в этом Perl-коде?

Question

Я написал некоторый код для печати отформатированных массивов (первая строка = нет входов, вторая строка = максимальная ширина чисел).Звезда может быть маркером любого типа, чтобы отличать некоторые элементы от остальных.

$ 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);

Я уже проверил это с помощью Perl :: Critic но это только скажет мне синтаксические проблемы (которые я уже исправил).Есть ли проблемы с этим кодом?Что-то, что опытный программист на Perl сделает по-другому?

Любые комментарии и предложения приветствуются.

Answer 1

Несколько предложений здесь.Надеюсь, что это полезно.

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;
}

Answer 2

• Операторы возврата не появляются в коде большинства людей - подпрограмма возвращается, когда она достигает конца (но см. Обсуждение в комментариях).

• В printwall я безоговорочно напечатал бы первую левую стену вне петли; то же самое с другими функциями.

• Я не уверен, что прочитал бы все данные в @inp, как показано. Скорее всего, я бы использовал:

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

  Это очистит функции $inp[$_ + 2].

• Я бы, вероятно, передал массив функциям, а не использовал бы глобальные переменные - глобалы в Perl грязные, как и везде.

• Подсчет количества значений не требуется на входе. С массивом, содержащим только данные для печати, вы можете перебирать каждый член массива в функциях с подходящим значением foreach, улучшая его Perlishness.

• В printnum вы можете создать строку формата один раз (не каждую итерацию).

• Это:

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

  - забавный способ написания:

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

• Я бы, вероятно, использовал регулярное выражение, чтобы найти звезду; неясно, следует ли вам печатать '*', если найден какой-либо символ, или вы должны напечатать найденный символ.

• Я бы, вероятно, использовал один формат для работы со звездами:

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

  Мне может понадобиться настроить одно из значений $ wid, чтобы учесть четную ширину, но результат будет:

  " %c  |"
  

  Затем можно просто напечатать каждую ячейку с пробелом или '*' для значения в формате.

• Точно так же в printnum я буду генерировать строку простого формата, такую ​​как " %2d |", чтобы напечатать каждое число - и я сгенерирую этот формат один раз.

1059 * Etc. *

Answer 3

Здесь есть много возможностей для совершенствования (я буду обновлять этот ответ, когда и у меня будет время).

Давайте начнем со входов . Вам не нужно указывать количество записей или их максимальную длину, поскольку Perl может сделать вывод, что для вас:

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

---

Не забудьте про CPAN.

Например, рассмотрим Text::ASCIITable.