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 have the following script in Perl that does the following:

  1. Prompts user for time in epoch. If user enters '########', the query will look for all reports generated after that date/time. If user does not input time, it will just look for all reports in past day.

  2. Connects to a database, runs a query to receive reports

  3. I create a directory of the current date (i.e. Jun25)

  4. I iterate through every row in the results and I save the contents of the xml file into an actual file and name the file [customer name]-[epoch time].xml

I am trying to clean up my code and have it run more efficiently.

#!/usr/bin/perl

#use strict;
use DBI;
use File::Slurp;
use Encode;
use utf8;
use POSIX qw(strftime);
use File::Slurp;
use XML::XPath;
use XML::XPath::XMLParser;
$| = 1;
print "Please enter epoch time of which to receive reports or leave blank to receive last day of reports: ";
my $epoch = <STDIN>;
chomp($epoch);
if ($epoch =~ /\D/) {$epoch = time() - 86400}
###Start Database
my $dsn = join "", ( 
    "dbi:ODBC:",
    "Driver={SQL Server};",
    "Server=SERVER;",
    "Database=DATABASE",
);
my $user = q/USERNAME/;
my $password = q/PASSWORD/;

my $db_options = {
    PrintError => 1,
    RaiseError => 1,
    #LongTruncOk => 1,
    LongReadLen => 100000000, 
};

print "CONNECTING TO DATABASE...";
# Connect to the data source and get a handle for that connection.
my $dbh = DBI->connect($dsn, $user, $password, $db_options)
    or die "Can't connect to $dsn: $DBI::errstr";
print "CONNECTED\nFETCHING ROWS AND PARSING...\n";

my $sql = "SELECT [receivedTime], [reportXml] from UsageReport WHERE [receivedTime] > " . $epoch . " AND [reportXml] NOT LIKE '%exclude%'";

# Prepare the statement.
my $sth = $dbh->prepare($sql)
     or die "Can't prepare statement: $DBI::errstr";

$sth->execute();
###End Database

#Make Directory of Current Date
$datestring = strftime "%b%d", localtime;
mkdir $datestring;
chdir $datestring;
# Fetch and display the result set value.
my @matrix;
my $count = 0;
while (my @row = $sth->fetchrow_array ) {
    my ($time, $report) = @row; #time, report
    $count++;
    eval {
        my $xp = XML::XPath->new(xml => $report);
        my $customer = $xp->getNodeText('/report/instances/instanceroot/instanceheader/customer');
        if ($customer eq "") {next;}
        our $ext = $customer . "-" . $time . ".xml";
        write_file($ext, $report);
    };
    if ( $@ ) {printf "ERROR ";}
    else {printf "$count ";}
}

# Disconnect the database from the database handle.
$dbh->disconnect;
print "\nCOMPLETED REPORT PARSING\nLatest Report Date: ";
share|improve this question
1  
Adding warnings and uncommenting strict may be a good idea. Also you can run profiler if you''re concerned with performance. You're checking return value for ->prepare but not for ->execute (when connecting RaiseError => 1 should do these checks for you). printf and our doesn't make sense here. –  mpapec Jun 26 at 12:18

1 Answer 1

up vote 2 down vote accepted

The program asks to input the time first, before trying to connect to the database. If the database connection fails, the user might be frustrated for having entered something only to watch the program crash. It would be better to move the input part right before it's really needed.

There are some unused elements:

  • use File::Slurp; appears twice, delete the second
  • @matrix is never used

The comment at the end of this line is pointless:

my ($time, $report) = @row; #time, report
share|improve this answer

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.