Perl - Mejora del código

Question

Acabo de empezar a codificar en Perl y solo estoy buscando si el código siguiente puede ser más eficiente o se puede hacer en menos líneas.

He investigado un poco sobre el módulo Win32::OLE y el módulo Text::CSV, pero este parecía ser el camino a seguir desde lo que he leído hasta ahora.

Esta pregunta es básicamente un novato que pregunta a un anciano: "Oye, ¿cómo me convierto en un mejor programador de Perl?"

El objetivo del código es obtener datos de rangos especificados en hojas especificadas de un libro de Excel y escribir el contenido de esos rangos en archivos CSV.

Además, sé que necesito implementar comprobaciones generales, como asegurarse de que mi $cellValue esté definido antes de agregarlo a la matriz y tal, pero estoy buscando más estructura general. ¿Hay alguna manera de aplanar el bucle colocando toda la fila en una matriz a la vez, o todo el rango en una matriz o referencia, o algo de esa naturaleza?

Gracias

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

Answer 1

La sugerencia de Mark es excelente. Otra mejora menor sería reemplazar "Haz un montón de lógica anidada if $cell, con" do not do anything unless $cell - de esta manera tienes un código ligeramente más legible (elimina 1 sangría adicional/bloque anidado; Y no tienes que preocuparte por qué sucede si $ celda está vacía.

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

---

Como se anotó, Text::CSV sería una buena cosa a considerar, dependiendo de si sus datos cada vez tiene que ser citado basado en el estándar CSV (por ejemplo, contiene espacios, comas , cotizaciones, etc.). Si es necesario citarlo, no reinvente la rueda y use Text::CSV para imprimir. Ejemplo no probado sería algo como esto:

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

Answer 2

No hay razón para que tengan bucle interno:

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

También, asegúrese de que tiene los índices correctos. No estoy familiarizado con Spreadsheet :: XLSX, así que asegúrese de que la fila col max & esté basada en cero como el resto de su código. Si no lo están, querrá iterar sobre 0 .. $maxCol-1.

Answer 3

No recomendaría la codificación difícil de nombres de archivos ... especialmente en proyectos pequeños como este, hágase el hábito de pasar los nombres de los archivos a través de GetOpt::Long. Si lo hace habitualmente con todos sus proyectos pequeños, es mucho más fácil recordar hacerlo bien cuando cuenta con un proyecto más grande.

Su código está bien estructurado y es legible, y anticipó los problemas con sus declaraciones de bucle, utilizó advertencias y es estricto, y generalmente está utilizando las bibliotecas de la manera correcta.

Answer 4

Como han dicho otros, su código es claro y está bien estructurado. Pero creo que podría mejorarse con un poco más Perlishness.

Los siguientes puntos vienen a la mente

• Use filehandles léxicas y la forma de tres parámetros de open (open my $newfile, '>', $myFile)

• iterar sobre valores de hash o una matriz (o rebanadas de ellos) en lugar de sus claves o índices, a menos que realmente necesite las claves para el cuerpo del bucle

• Extraiga los punteros a los datos s ubstructures dentro de un bucle si ese es el foco del bucle (my $rows = $worksheet->{Cells})

• lugar donde se está utilizando un bucle para transformar una lista a otra, y el uso map lugar

espero que no he T saltó la pistola un poco al escribir una solución utilizando Text::CSV como usted propuso. Con suerte, esto es instructivo para ti.

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