Script for performing tasks with acquired XML files

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"){
@

-
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.

-
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.

-
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?

-
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.

StackExchange Code Review Q#4244, answer score: 5