Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am very new to Perl, but quite pleased with my short introduction to it!

So, scenario is I have a number of text files (they are reports from a financial system) stored in a directory. Each report has a particular piece of text to search for (eg Report Total) and then a range of characters to extract for the value (eg position 78 for 10 characters). There are a number of these reports - but no more then 200 so not a huge amount of processig or work required.

The premise of my solution is to maintain a simple CSV file with the report name, text to search for, start value for extract, length of characters to extract. Then. it is a simple case of updating this text file to add or remove the reports as necessary.

The script I have developed is below, and it works, but I would be interested in some critique as to how it may be made "better" (your definition :-) )

I have to admit that some of it I do not fully understand, but lots of searching on here has provided me guidance on where I needed asisstance - so thanks to evryone who contributes here...

So, what needs changeing/improving and why (bear in mind this is 5.8.5 so v.v.v.v.v. old!?

#!/usr/bin/perl -w
use strict;
use strict;
use Data::Dumper;
use Cwd;
use Text::CSV;

my $SrcDir="/data03/adudata/live/work/";
my $RepDir="/data03/adudata/live/archive/";
my $ReportList='pl_rec.list';
my $csv = Text::CSV->new();
my @FileToRead;
my @TextToFind;
my @RowsOffset;
my @DataStart;
my @DataEnd;
my %ReportData;

chdir $SrcDir;
open my $fh, '<', $ReportList or die "Could not open file";
while (my $row = $csv->getline( $fh )) {
  push (@FileToRead, $row->[0]);
  push (@TextToFind, $row->[1]);
  push (@RowsOffset, $row->[2]);
  push (@DataStart, $row->[3]);
  push (@DataEnd, $row->[4]);
}

close ($fh);

for my $i(0 .. $#FileToRead) {
  open $fh, "<", $RepDir.$FileToRead[$i]. ".prn" or
    die "Could not open $FileToRead[$i]";

  while (<$fh>) {

    if (grep /$TextToFind[$i]/, $_) {
      my $extractValue = substr($_,$DataStart[$i],$DataEnd[$i]);
      $extractValue =~s/(^\s+)|(s+$)//g;
      $extractValue =~ s/,//g;
      $ReportData{$TextToFind[$i]} = $extractValue+0;
    }
  }
close $fh;
}

while(my($code,$value)=each(%ReportData)){
  print "$code,$value\n";
}
share|improve this question

migrated from stackoverflow.com Dec 5 '13 at 5:58

This question came from our site for professional and enthusiast programmers.

add comment

2 Answers 2

The point of filehandles is that it allows you to open more than one file simultaneously. So you could be reading the file list and the files you're searching through simultaneously.

Also, this:

if (grep /$TextToFind[$i]/, $_)

Is a a really convoluted way to write:

if (/$TextToFind[$i]/)

Overall, I'd do something like this:

#!/usr/bin/env perl

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

my $SrcDir = "/data03/adudata/live/work/";
my $RepDir = "/data03/adudata/live/archive/";
my $ReportList = 'pl_rec.list';

open my $list_fh, '<', $SrcDir.$ReportList
  or die "Could not open $ReportList: $!";

my %ReportData;
my $csv = Text::CSV->new();

while (my $row = $csv->getline( $list_fh ))
{
  my ($filename, $text, $rows_offset, $data_start, $data_end) = @$row;

  open my $fh, '<', $RepDir.$filename
    or die "Could not open $filename: $!";

  while (<$fh>)
  {
    next unless /\Q$text_to_find/;

    my $extractValue = substr($_, $data_start, $data_end);
    $extractValue =~ s/(^\s+)|(s+$)//g;
    $extractValue =~ s/,//g;

    $ReportData{$text} = $extractValue + 0;
  }
}

for (sort keys %ReportData)
{
  my $value = $ReportData{$_};
  print "$_,$value\n";
}
share|improve this answer
    
Many thanks for the time to respond :) and for the pointers! I'll add those to my learnings! –  Daz Dec 5 '13 at 16:35
add comment

This is pretty good for beginner code.

Stating use strict twice does not make Perl any more strict. ☺ You have some extraneous requirements for Cwd and Data::Dumper.

A more proper way to append a filename to a directory is to use File::Spec->catfile().

Since you operate on each row of the CSV file independently, there's no need to build up arrays like @FileToRead — just do the processing as you progress through the CSV.

Strings interpolated inside a regular expression are treated as regular expressions. That is, if $TextToFind contains any "special regex characters" (e.g. .), they are not treated literally (e.g. match any character). If you want to do a string match for $TextToFind rather than a regular expression match, you can quote it as /\Q$TextToFind\E/.

You can strip whitespace and commas in one substitution. You missed a backslash when specifying the trailing whitespace.

#!/usr/bin/perl -w

use strict;
use File::Spec;
use Text::CSV;

my $ReportList = 'pl_rec.list';
my $SrcDir = "/data03/adudata/live/work/";
my $RepDir = "/data03/adudata/live/archive/";

my %ReportData;

sub process_row {
    my ($FileToRead, $TextToFind, $RowsOffset, $DataStart, $DataEnd) = @_;
    open my $fh, "<", File::Spec->catfile($RepDir, "$FileToRead.prn")
      or die "Could not open $FileToRead";

    while (<$fh>) {
        if (grep /\Q$TextToFind\E/, $_) {
            my $extractValue = substr($_, $DataStart, $DataEnd);
            $extractValue =~ s/(^\s+)|,|(\s+$)//g;
            $ReportData{$TextToFind} = $extractValue + 0;
        }
    }
    close $fh;
}

open my $fh, '<', File::Spec->catfile($SrcDir, $ReportList)
  or die "Could not open file";
my $csv = Text::CSV->new();
while (my $row = $csv->getline($fh)) {
    process_row(@$row);
}
close ($fh);

while (my ($code,$value) = each(%ReportData)) {
    print "$code,$value\n";
}
share|improve this answer
    
I hadn't noticed the double 'strict' declaration - would be funny if it DID make it extra strict!! As above, many thanks for taking the time to respond and show me different ways of achieving the result! –  Daz Dec 5 '13 at 16:36
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.