3
\$\begingroup\$

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: ";
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – mpapec Jun 26 '15 at 12:18
3
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

You should use DBI prepared statements to avoid injecting a variable into your SQL statement, as this variable content is directly controlled by the user of your program.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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