Script for performing tasks with acquired XML files

Question

I have been working on a script for almost a month now, which takes in an XML file and uses it to either download or upload files from an ftp site. It also copies the files to a history directory for historical purposes.

This thing has to be flawless so any suggestions to make the script more stable, reliable, error free (should able to capture errors and continue with out ending the script just because one file was not found). Eventually there will be an email function to let me know of any errors and warnings. I will also me updating a database with information but more on that later here is the script and a sample XML file.

Perl

#! /usr/bin/perl -w
use DBI;
use strict;
use Switch;
use Net::FTP;
use Net::FTP::File;
use File::Copy;
use Getopt::Long;
use File::Basename;
use XML::Simple qw(:strict);
use Mail::Sender::Easy qw(email);

my ($e, $w, $p);
my (@files, @history);
my (%failed, %delete, %missing);
my ($config, $options, $xml, $ftp, $sbj, $msg, $error, $fi);

$options = GetOptions ("config=s" => \$config);
unless(-e $config){print "Could not find $config.\n";exit;}
$xml = XMLin($config, ForceArray=>0, KeyAttr=>[]);

if(exists $xml->{files}->{file}){
if (ref($xml->{files}->{file}) eq ""){
unless(-e $xml->{files}->{file}){$missing{$xml->{files}->{file}} = $!;next;}
if(lc($xml->{type}) eq "upload"){
    push(@files, "$xml->{localpath}/$xml->{files}->{file}");
}
elsif(lc($xml->{type}) eq "download"){
    push(@files, $xml->{files}->{file});
}
}
if(ref($xml->{files}->{file}) eq "ARRAY"){
if(lc($xml->{type}) eq "upload"){
    foreach $fi ((@{$xml->{files}->{file}})){
        unless(-e $fi){$missing{$fi} = $!; next;}
        push(@files, "$xml->{localpath}/$fi");
    }
}
elsif(lc($xml->{type}) eq "download"){
    foreach $fi ((@{$xml->{files}->{file}})){
        push(@files,$fi);
    }
}
} 
}
elsif(exists $xml->{files}->{regex}){
if(lc($xml->{type}) eq "upload"){
@files = glob("$xml->{localpath}/$xml->{files}->{regex}");
}
elsif(lc($xml->{type}) eq "download"){
@files = ();
}
}
else{print "No files specified\n";}

$ftp = Net::FTP->new($xml->{host});
if($@ ne ""){$e=1;}
unless ($ftp->login($xml->{user}, $xml->{password})){print $ftp->message;}
unless($ftp->cwd($xml->{serverpath})){print $ftp->message; exit;} 

switch ($xml->{type}){    
case "upload"{       
foreach my $file (@files){
    if(exists $xml->{name}){
        unless($ftp->put($file, $xml->{name})){$failed{$file} =        $ftp->message;next;}
    }
    else{
        unless($ftp->put($file)){$failed{$file} = $ftp->message;next;}
    }
    push(@history, $file) if(exists $xml->{historypath});
}
}
case "download"{
if(scalar(@files) > 0){
    foreach my $file (@files){
        unless($ftp->exists($file)){$missing{$file} = "not found on server.";next;}
        unless(exists $xml->{name}){$ftp->get($file, "$xml->{localpath}/$xml->     {name}");}
        unless($ftp->get($file, "$xml->{localpath}/$file")){$failed{$file} =    $ftp->message; next;}
    }
}
elsif(scalar(@files) == 0){
    @files = $ftp->ls($xml->{files}->{regex}) if($xml->{files}->{regex});
}
}
else{print "job type not defined.\n";}
}
$ftp->quit;
if( @history > 0 ){
if($xml->{date} == 0){
foreach my $h(@history){
    unless(copy($h, "$xml->{historypath}/")){$delete{$h} =  $!; next};
    unlink($h) if($xml->{del} == 1);
}
}
elsif($xml->{date} == 1){
foreach my $h(@history){
    (my $n, my $p, my $s) = fileparse($h, qr/\.[^.]*/);
    use POSIX qw(strftime);
    my $file = $n . "-" . strftime("%b-%e-%H-%M", localtime) . $s;
    unless(copy($h,"$xml->{historypath}/$file")){$delete{$h} = $!; next;}
    unlink($h) if($xml->{del} == 1);            
}

 }
 }

while ((my $k, my $v) = each %missing){print "$k => $v\n";}
while ((my $k, my $v) = each %failed) {print "$k => $v\n";}
while ((my $k, my $v) = each %delete) {print "$k => $v\n";}

XML

<?xml version="1.0" encoding="ISO-8859-1"?>
<site>
    <job>zenadown</job>
    <type>download</type>
    <host>zena.leegin.com</host>
    <ip>128.222.79.75</ip>
    <user>leo</user>
    <password>green97</password>
    <serverpath>/leo</serverpath>
    <localpath>/temp</localpath>
    <historypath>/temp/leo</historypath>
    <files>
        <file>jo.png</file>
        <file>jo2.png</file>
        <file>jo3.png</file>
        <file>jo4.png</file>
    <file>Blogger.png</file>
    </files>
    <logfile>/zena/AUTOMIZE/leotest.txt</logfile>
    <del>0</del>
    <date>0</date>
</site>

Answer 1

1. Please indent consistently. It makes it easier for others to read your code, and it makes it easier for you to read your code, preventing mistakes. Seeing three close braces in a row lined up along the left side of the terminal makes me sad.

2. Don't use Switch. Use given/when, or use if/elsif chains as you're doing elsewhere in your code, or use dispatch tables (hashes containing code references), or use OO methods called by name, but don't use Switch. It makes your code harder to debug, and sometimes it will cause your code to simply stop working.

3. You're using ForceArray => 0 and then switching on whether the values you find are arrayrefs or plain scalars, and writing a lot of duplicate code, in a situation where (as far as I can tell) you could simply use ForceArray => 1 and shave off half of the code. Why?

4. It's hard to tell at a glance what the code does or why, and that's a bad sign for reliability. Try splitting it up into functions along functional lines -- for example, one to parse the information you need from the XML file, one to perform uploads, one to perform downloads, and one to present the results to the user. Give each function documentation that says what it expects as input, what it produces as output, how it behaves in case of errors, etc. A program like yours should have no more than a dozen lines of code outside of functions. In fact, you should probably have half that many.

Answer 2

Adding to what @hobbs has said:

1. Indentation: get perltidy to help format your code.

2. Switch versus given/when: as long as your code has use v5.10; given/when makes for a better alternative than the Switch module. Also, when is also useful in for loops, and I can see some places in your code that could be made clearer using a few loops.

3. XML::Simple may not be even a good fit here. That module does provide a simple interface, but it is tailored more for reading XML config files than what looks like an XML response above. Maybe a combination of XML::LibXML (for parsing to DOM) and XML::LibXML::XPathContext (for navigating that DOM) would do better here, (not to mention XML::LibXML is faster, being built on top of libxml2.)

4. Your script looks to be a controller of some sort: it takes XML input and decides on whether to upload or download files given the instructions parsed there. Might be better to break up the big parts into modules (say, one module for the XML parsing and another for file upload/download, and eventually one more for the mailer/error handler.)

Answer 3

The most obvious change you should make is to add some comments and, if you don't have it elsewhere, some documentation!

Typical documentation is designed for the user - that might be an end user, or it might be another programmer who simply wants to use your code as a "black box" library. Comments are documentation for the programmer who needs to alter or debug your code. That person might be you a few months down the line, or it might be someone else. Good comments briefly describe the algorithm, so they provide additional information to the user-level documentation, which should describe the input and output.

Even thought this is your code and you're intimately familiar with it, you'd be surprised how hard it can be to understand what it does after a few months away from it. Good comments and documentation mitigate this.

Answer 4

Use more descriptive variable names. Seeing the global variables my ($e, $w, $p); at the top of your script gave me the heebie-jeebies. Using $i as a counter in a short loop may be acceptable, otherwise you should use variable names which actually describe what they contain.

As a general rule, I try to write a full description of the code that I'm going to write, then take the variable names from that description.

Answer 5

Single letters are almost never good variable names. case in point:
my ($e, $w, $p);

There is not a single comment in this script, yet this script is highly complex and difficult to figure out. Try to explain a bit what you are doing each time.
Try to break the problem down into a few smaller problems and note those problems down. Often doing that leads to reuse of code.
There is no design of this code, meaning nowhere can i see what it is that you are trying to do, and then how you intend to do it. Noting that down will make for an easy to read overview of what you are doing. That would also be very helpful if ever the solution needs to be implemented in another programming language. Or if what you are doing needs to be explained to.. anyone.