perl - Perl - 代码审查

Question

我正在开发一个程序，该程序从 CSV 文件中获取信息作为源，以通过具有“客户包”的文本文件进行搜索。我只对某些条目进行了奇怪的计数，我似乎无法弄清楚是什么导致了重复计数。谁能查看我的代码并告诉我我的逻辑/语法是否关闭？（可能是）。我想要完成的只是计算 csv 文件中条目的文本文件中的总出现次数（packageid，package_description）

谢谢您的帮助！我在这里发疯了。

#!/usr/bin/perl

use strict;
use Text::CSV;

# Variables already declared in the other PL file ** Remove if consolidating **

my $file2 = 'master_plist.csv';
my $csv2 = Text::CSV->new(); # Create a Text::CSV object

open (CSV2, "<", $file2) or die $!; #open CSV file for parsing

while (<CSV2>) {

    if ($csv2->parse($_)) {
            my @columns2 = $csv2->fields(); # Parse CSV and load into an array for each row.
            my $packID = $columns2[0];
            my $packDESC = $columns2[1];



my $val = 'customer_packages_report.txt';

chomp ($val);

my $cnt=0;

open (HNDL, "$val") || die "wrong filename";

while ($val = <HNDL>)
{
while ($val =~ /$packID - $packDESC/ig)
{
    $cnt++;
}
}

#if ($packDESC =~ /\(/g) {
#       $packDESC =~ s/\(/\(/g;
#} 
print "Total iterations of $packDESC: $cnt\n";

close (HNDL);
# End original code

    } # Close IF
} # Close WHILE

close CSV;

score 2 · Accepted Answer

#!/usr/bin/perl

use strict;
use warnings;
use Text::CSV;

# Variables already declared in the other PL file ** Remove if consolidating **

my $file2 = 'master_plist.csv';
my $csv2 = Text::CSV->new(); # Create a Text::CSV object

open (CSV2, "<", $file2) or die "I die while opening $file2!  $!"; #open CSV file for parsing

while ($each_csv2_line=<CSV2>) {

    if ($csv2->parse($each_csv2_line)) {
            my @columns2 = $csv2->fields(); # Parse CSV and load into an array for each row.
            my $packID = $columns2[0];
            my $packDESC = $columns2[1];



            my $val = 'customer_packages_report.txt';

            chomp ($val);

            my $cnt=0;

            open (HNDL,"<","$val") or die "wrong filename: $val! $!";

            while (<HNDL>){
                $cnt++ while (/$packID - $packDESC/ig);
            }

#if ($packDESC =~ /\(/g) {
#       $packDESC =~ s/\(/\(/g;
#} 
            print "Total iterations of $packDESC: $cnt\n";

            close (HNDL);
            # End original code

    } # Close IF
} # Close WHILE

# end of script
close CSV;

我的建议：

$HNDL instead of HNDL更好地为文件句柄使用<- 词法变量。
尝试捕捉所有错误（通过definedand==0和eq ""）
我尝试格式化您的代码并添加一些我有时使用的功能。比我更好，首先阅读Little Perl Monk 的 Style Coding。而且您可以使用这种语言更令人印象深刻，并且不仅可以编写writeonly代码。

示例（以及引用）：

“对于行输入操作符，情况完全相同<>，尽管 Perl 会自动为您执行此操作。
看起来您正在测试来自 STDIN 的行，而此时：

    while (<STDIN>) {
       do_something($_);
    }

然而，这是 Perl 自动转换以检查定义性的一种特殊情况$_：

     while ( defined( $_ = <STDIN> ) ) {  # implicitly done
       do_something($_);
     }

" 有效的 Perl 编程，第 24 页。

score 2 · Accepted Answer

你可以做很多事情来改进你的代码：

use warnings;.
使用适当的缩进。
使用描述性变量名称。而不是$file2（没有意义，为什么没有文件1？），使用$package_file或任何有意义的东西。
如果您已经在使用Text::CSV，您可以使用$csv->getline()逐行浏览文件。这将简化您的代码。有关示例，请参阅文档。
chomp($val)从字符串末尾删除换行符。您在刚刚声明的没有换行符的字符串文字上使用它。那没有意义。
永远不要使用同一个变量 ( $val) 来做两件完全不同的事情。这非常令人困惑。
您在正则表达式中插值的变量可能包含特殊字符吗？如果是这样，您需要逃脱它们。例如，如果$packDESC包含句点，它将匹配正则表达式中的任何字符。要按字面意思处理变量的内容，请使用\Q..\E，如本例所示：/\Q$packID - $packDESC\E/ig.
您正在打开 customer_packages_report.txt 并在 csv 文件的每一行上逐行浏览它。您可以通过一次读取并将结果存储在数组中来简化此操作。
您不需要 while 循环来计算匹配项：$cnt = () = /$packID - $packDESC/ig;。这会将匹配项放入数组上下文中，返回一个匹配数组，然后将其放回标量上下文中以计算匹配项。有点棘手，但更简单。

如果没有看到数据，很难确切地说出是什么导致了您的问题。您可能有一些不必要的重复，这些重复源于您对两个文件的嵌套循环？我将首先重写以改进您的代码，然后查看问题是否仍然存在。

score 1 · Accepted Answer

您的代码似乎编译时perl -c没有错误，这很好。如果我猜测，我会假设您的问题在于您的某些字段中有元字符。正则表达式/$packID - $packDESC/容易受到元字符的影响。例如

my $str = "foo? bar";
$str =~ /$str/;       # returns false, because ? is a meta character

在上面的例子中，问号?是一个量词，它影响它之前的任何东西，所以它的o?意思是“0 或 1 o”。要解决元字符问题，请使用\Q ... \E转义：

$str =~ /\Q$str/;   # will now match

终止转义序列\E是可选的。

其他一些需要注意的事项：

您使用它非常好use strict。你也应该永远use warnings。不这样做不会消除代码的问题，只会隐藏它们。
Text::CSV您使用默认设置创建对象。根据您的输入，这可能合适也可能不合适。文档binary => 1中推荐设置。
使用该parse()功能可能不是最好的选择，文档中有很多关于getline.
正如loldop在评论中指出的那样，您正在重用$val从文件中读取。虽然在技术上应该可行，但它正在自找麻烦。

风格和练习笔记和实用技巧：

使用三参数打开和词法文件句柄是一件好事。三参数本质上意味着使用显式打开模式，这使您的脚本使用起来更安全。使用词法文件句柄意味着您的文件句柄将没有全局范围，这是一件好事。
这段代码

my @columns2 = $csv2->fields(); 
my $packID = $columns2[0];
my $packDESC = $columns2[1];

可以这样写

my ($packID, $packDESC) = $csv2->fields();

$val分配它后，您正在咀嚼。这是多余的，因为chomp默认情况下只从字符串末尾删除换行符，而您没有添加任何此类。它不会改变任何东西，但这里不需要。但是，如果您从标准输入或文件中读取某些内容，您可能希望使用chomp.
die在不提及错误的情况下使用$!是让自己生气的可靠方法。
不要低估使用适当的缩进编写代码会变得多么容易。使用具有自动缩进和着色功能的文本编辑器。我可以热烈推荐 vim（如果您使用的是 windows，请使用 gvim）。虽然它有一个学习曲线，但它是一个功能强大的编辑器，通常也已经安装在许多系统上。

score 1 · Accepted Answer

由于已经有很多人对您的程序本身发表了评论，我将讨论如何成为一名更好的 Perl 程序员，并帮助您以一种有助于消除您的许多问题的方式编写代码。

看看Perl::Tidy并彻底运行你的程序。这将有助于改进您的语法和 Perl，并将帮助您发现许多您遇到的各种问题。

此外，您应该获得一份Perl Best Practices的副本，这是大部分 Perl Tidy 的来源。而且，正如有人已经提到的，Effective Perl Programming是另一本优秀的书。

Perl 最大的问题是很少有人学习它。大多数人都陷入了我们不得不自己捡起来的境地。另外，Perl 是一种相当古老且相当粗糙的语言。大多数 Perl 书籍仍然严重依赖 Perl 3.x 的编程方式，而没有提及使用use strict;和use warnings;.

您将旧的编程实践结合起来，大多数人通过破解旧程序和旧语法来学习 Perl（并且可能是由通过破解甚至更旧的程序来学习 Perl 的人编写的），您就会明白为什么 Perl 享有盛誉作为一种只写语言。

score 0 · Accepted Answer

您可能想使用getlinefrom 方法Text::CSV，它可以节省几行代码。

问题可能是因为您正在搜索的字符串中有正则表达式元字符。在正则表达式中转义它们，\Q...\E以便按字面意思理解它们。在下面的重写中，我还添加了\s*而不是文字空格，以防连字符的任一侧都没有一个空格。

我还将文件句柄更改为词法句柄，其优点是当句柄超出范围时它们将自动关闭。

#!/usr/bin/perl

use strict;
use warnings;

use Text::CSV;

my $file2 = 'master_plist.csv';
my $csv2  = Text::CSV->new();

open(my $csv_fh, '<', $file2) or die $!;

while (my $row = $csv2->getline($csv_fh)) {

  my ($packID, $packDESC) = @$row;

  my $val = 'customer_packages_report.txt';
  chomp($val);

  open(my $fh, '<', $val) or die "wrong filename";
  my $cnt = 0;
  while ($val = <$fh>) {
    while ($val =~ /\Q$packID\E\s*-\s*\Q$packDESC\E/ig) {
      $cnt++;
    }
  }

  print "Total iterations of $packDESC: $cnt\n";
}

perl - Perl - 代码审查

5 回答 5

Related

Reference