Perl - Code Enhancement

Question

Ho appena iniziato la programmazione in Perl e sto solo cercando di scoprire se il codice sottostante può essere reso più efficiente o può essere fatto in meno righe.

Ho studiato un po 'il modulo Win32::OLE e il modulo Text::CSV, ma questo sembrava il modo di passare da quello che ho letto finora.

Questa domanda è fondamentalmente un principiante che chiede a un anziano: "Ehi, come faccio a diventare un programmatore Perl migliore?"

Lo scopo del codice è ottenere dati da intervalli specificati in fogli specifici di una cartella di lavoro di Excel e scrivere il contenuto di tali intervalli in file CSV.

Inoltre, so che ho bisogno di implementare controlli generali, come ad esempio assicurarmi che il mio $cellValue sia definito prima di aggiungerlo all'array e così via, ma sto cercando di più per la struttura generale. Come c'è un modo per appiattire il loop mettendo tutta la fila intera in una matrice in una volta, o l'intera gamma in una matrice o riferimento, o qualcosa di quella natura?

Grazie

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

Il suggerimento di Marco è eccellente. Un altro piccolo miglioramento sarebbe quello di sostituire "Fai un mucchio di logica nidificata if $cell, con" non fare nulla unless $cell - in questo modo hai un codice leggermente più leggibile (rimuovi 1 indentazione aggiuntiva/blocco nidificato, E non devi preoccuparti di cosa succede se $ cella è vuota.

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

---

Come annotato, Text::CSV sarebbe una buona cosa da considerare, a seconda se i dati devono mai di essere citato in base allo standard CSV (per esempio, contiene spazi, virgole , citazioni ecc ...). Se può essere necessario citare, non re-inventare la ruota, e utilizzare invece Text::CSV per la stampa. Esempio non testato sarebbe qualcosa di simile:

# 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

alcun motivo per avere quel ciclo interno:

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

Inoltre, assicuratevi di avere gli indici corretti. Non ho familiarità con Spreadsheet :: XLSX, quindi assicurati che la colonna max col & sia zero come il resto del tuo codice. Se non lo sono, ti consigliamo di eseguire l'iterazione su 0 .. $maxCol-1.

Answer 3

Vorrei consigliare contro nomi di codifica dei file duri ... soprattutto nei piccoli progetti come questo, prendere l'abitudine di passare i nomi dei file in via GetOpt::Long. Se lo fai abitualmente con tutti i tuoi piccoli progetti, è molto più facile ricordarsi di farlo correttamente quando conta su un progetto più grande.

Il tuo codice è ben strutturato e leggibile, e hai anticipato i problemi con le tue istruzioni di loop, hai usato avvertenze e severi, e generalmente stai usando le librerie nel modo giusto.

Answer 4

Come altri hanno già detto, il codice è chiaro e ben strutturato. Ma penso che potrebbe essere migliorato con un po 'più di Perlishness.

I seguenti punti vengono in mente

• Usa filehandle lessicali e forma tre parametri di open (open my $newfile, '>', $myFile)

• iterare valori hash o array (o fette di loro) piuttosto che le loro chiavi o indici, a meno che non sia realmente necessario le chiavi per il corpo del ciclo

• Puntatori di estrazione ai dati s ubstructures all'interno di un ciclo se che è al centro del ciclo (my $rows = $worksheet->{Cells})

• punto in cui si utilizza un ciclo di trasformare una lista in un altro, e utilizzare map invece

spero di rifugio' t saltato un po 'la pistola scrivendo una soluzione usando Text::CSV come da te proposto. Con fortuna questo è istruttivo per te.

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