7

我刚刚开始使用 Perl 进行编码，我只是想看看下面的代码是否可以变得更有效率或者可以用更少的行来完成。

我已经对Win32::OLE模块和Text::CSV模块进行了一些研究，但这似乎是我目前所读内容的方式。

这个问题基本上是一个新手问一个长辈：“嘿，我如何成为一个更好的 Perl 程序员？”

该代码的目的是从 Excel 工作簿的指定工作表中的指定范围获取数据，并将这些范围的内容写入 CSV 文件。

另外，我知道我需要执行一般检查，例如确保在将 my$cellValue添加到数组之前定义它等，但我正在寻找更多的整体结构。就像有没有办法通过一次将所有整行放入一个数组，或者一个数组或引用中的整个范围，或者类似的东西来使循环变平？

谢谢

use strict;
use warnings;
use Spreadsheet::XLSX;

my $excel = Spreadsheet::XLSX -> new ('C:\scott.xlsm',);
my @sheets = qw(Fund_Data GL_Data);

foreach my $sheet (@sheets) {

    my $worksheet = $excel->Worksheet($sheet);
    my $cell = $worksheet->get_cell(25,0);

    if ($cell) { # make sure cell value isn't blank
        my $myFile = "C:/$sheet.csv";
        open NEWFILE, ">$myFile" or die $!;

        # write all cells from Range("A25:[MaxColumn][MaxRow]") to a csv file
        my $maxCol = $worksheet->{MaxCol};
        my $maxRow = $worksheet->{MaxRow};
        my @arrRows;
        my $rowString;

        # loop through each row and column in defined range and string together each row and write to file
        foreach my $row (24 .. $maxRow) {

            foreach my $col (0 .. $maxCol) {

                my $cellValue = $worksheet->{Cells} [$row] [$col]->Value();

                if ($rowString) {
                    $rowString = $rowString . "," . $cellValue;
                } else {
                    $rowString = $cellValue;
                }
            }

            print NEWFILE "$rowString\n";
            undef $rowString;
        }
    }
}
4

4 回答 4

6

没有理由有那个内部循环：

print NEWFILE join(",", map { $worksheet->{Cells}[$row][$_] } 0 .. $maxCol), "\n";

另外，请确保您的索引正确。我对 Spreadsheet::XLSX 不熟悉，因此请确保 max col & row 与其他代码一样从零开始。如果不是，那么您将需要遍历0 .. $maxCol-1.

于 2012-05-24T20:33:27.233 回答
6

马克的建议是一个很好的建议。另一个小的改进是将“做一堆嵌套逻辑”替换为if $cell“不做任何事情unless $cell- 这样你的代码可读性更高（删除 1 个额外的缩进/嵌套块；并且不必担心如果$cell 为空。

# OLD
foreach my $sheet (@sheets) {
    my $worksheet = $excel->Worksheet($sheet);
    my $cell = $worksheet->get_cell(25,0);

    if ($cell) { # make sure cell value isn't blank
        # All your logic in the if
    }
}

# NEW
foreach my $sheet (@sheets) {
    my $worksheet = $excel->Worksheet($sheet);
    next unless $worksheet->get_cell(25,0); # You don't use $cell, so dropped

    # All your logic that used to be in the if
}

正如您所指出的，Text::CSV这将是一件好事，这取决于您的数据是否需要根据 CSV 标准进行引用（例如，包含空格、逗号、引号等......）。如果可能需要引用，不要重新发明轮子，Text::CSV而是用于打印。未经测试的示例将是这样的：

# At the start of the script:
use Text::CSV;
my $csv = Text::CSV->new ( { } ); # Add error handler!

    # In the loop, when the file handle $fh is opened
    foreach my $row (24 .. $maxRow) {
        my $cols = [ map { $worksheet->{Cells}[$row][$_] } 0 .. $maxCol) ];
        my $status = $csv->print ($fh, $cols);
        # Error handling
    }
于 2012-05-24T20:40:30.473 回答
4

我建议不要对文件名进行硬编码……尤其是在像这样的小型项目中，养成在 via 中传递文件名的习惯GetOpt::Long。如果你对所有的小项目都习惯性地这样做，那么当它依赖于一个更大的项目时，更容易记住正确地做这件事。

您的代码结构良好且可读性强，并且您预见到了循环语句的问题，您使用了警告和严格，并且您通常以正确的方式使用库。

于 2012-05-24T21:07:57.457 回答
4

正如其他人所说，您的代码清晰且结构良好。但我认为它可以通过更多的 Perlishness 来改进。

想到以下几点

  • open使用词法文件句柄和( open my $newfile, '>', $myFile)的三参数形式

  • 迭代哈希或数组值（或它们的切片）而不是它们的键或索引，除非你真的需要循环体的键

  • 如果这是循环的焦点，则提取指向循环内数据子结构的指针 ( my $rows = $worksheet->{Cells})

  • 找出您使用循环将一个列表转换为另一个列表的位置，然后map改用

Text::CSV我希望我没有像您建议的那样使用 using 编写解决方案。幸运的是，这对你很有启发性。

use strict;
use warnings;

use Spreadsheet::XLSX;
use Text::CSV;

my $csv = Text::CSV->new;

my $excel = Spreadsheet::XLSX->new('C:\scott.xlsm',);

foreach my $sheet (qw/ Fund_Data  GL_Data /) {

  my $worksheet = $excel->Worksheet($sheet);
  next unless $worksheet->get_cell(25,0);

  my $myFile = "C:\\$sheet.csv";
  open my $newfile, '>', $myFile or die $!;

  my $rows = $worksheet->{Cells};

  # Write all cells from row 25 onwards to the CSV file

  foreach my $row (@{$rows}[24..$#{$rows}]) {
    my @values = map $_ ? $_->Value : '', @$row;
    $csv->print($newfile, \@values);
    print $newfile "\n";
  }
}
于 2012-05-25T03:21:31.043 回答