这段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程序员会做得不同的地方？

欢迎任何评论或建议。

- Lazer

---

我还没有深入研究过，但是可扩展性怎么样？更具体地说，当遇到多维数组时会发生什么？我至少会允许2维数组，并且出于这个原因，你应该从上到下而不是从左到右打印。 - vol7ron

对于像“如何改进我的代码”这样的问题，可能比Stackoverflow更好的选择是http://refactormycode.com/。其中有一个Perl部分，需要更多的输入！http://refactormycode.com/codes/recent/perl - draegtun

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

- FMc

Answer 2

这里还有很多可以改进的地方（我会在有时间的时候更新这个答案）。

让我们从输入开始。您不需要指定条目数量或其最大长度，因为Perl可以为您推断：

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

---

别忘了CPAN。

例如，考虑Text::ASCIITable。

- Zaid

Answer 3

• 大多数人的代码中不会出现返回语句 - 子程序在到达结尾时返回（但请参见注释中的讨论）。

• 在printwall中，我会无条件地在循环外打印第一个左墙；其他函数也是如此。

• 我不确定是否应该像所示那样将所有数据读入@inp中。更可能的情况是：

  my $num = <STDIN>;  # 或者更可能的是 <>
  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 |"来打印每个数字 - 我会生成该格式一次。

等等。

- Jonathan Leffler

---

非常感谢，@Jonathan。1）我在阅读这篇文章后加入了返回语句。2）如果我先解析“星号”，然后再打印解析出来的内容，我认为会带来不必要的复杂性。在那种情况下，我需要处理很多错误情况。因此，我所做的是无论“星号”输入是什么都打印一个*。 - Lazer

@Lazer：关于Perl Critic x-ref（交叉引用）和显式返回，我想我是那些受到批评的人之一，尽管我从不会隐式地依赖于子程序中的最后一个表达式来提供返回值（如果我要返回值，我会使用return！）。 关于星号，我几乎肯定会使用一个子程序将数据拆分成数字和可选标记，并将这两个信息分别存储，可能在两个并行数组中。 我可以进行错误检查 - 例如，非数字行可以被拒绝为无效。 - Jonathan Leffler

顺便说一句：我的（新的）不返回任何值的子程序现在都明确以 return; 结尾 - 我活着，我学习。 - Jonathan Leffler