I don't know which error you get or what isn't happening, but I'ld like to give some comments on your code:
#!/usr/bin/perl
use XML::Simple;
print "Content-type: text/html\n";
print "\n";
######XML PARSE##########
$ipath = "e:/cars/";
$str = ".xml";
opendir THEDIR, "$ipath";
@xmlfile = grep (/$str/, readdir THEDIR);
closedir THEDIR;
foreach $file (@xmlfile){
$xml = $ipath.$file;
}
This bracket above confuses me. At this place, it would make the loop go through the directory getting a random filename (the last one readdir returns, but you can't say which one would be the last returned). The order could change over time, reboots, fsck's, OS-changes, directory copies...
Okay, I'll assume that you know what you're doing :-)
if (lc($xml) =~ "don"){&doncaster} else {
if (lc($xml) =~ "ayl"){$holding = "asmholding"};
if (lc($xml) =~ "cartrans"){$holding = "ctpholding"};
if (lc($xml) =~ "hills"){$holding = "hilholding"};
if (lc($xml) =~ "windley"){$holding = "wlsholding"};
if (lc($xml) =~ "ppm"){$holding = "ppmholding"};
The following processing only takes for file with doesn't include
don.
Using elsif for the last if-block might be better.
print "Other Cars<br>";
$config = XMLin( $xml, SuppressEmpty => "" );
$exportid = "$config->{Header}->{ExportID}";
$numveh = "$config->{Summary}->{NumberOfVehicles}";
$memid = "$config->{Header}->{MemberID}";
exportid and memid are being used, but not modified and numveh isn't used at all. You may want to use their long form (config-header-exportid, like you wrote) instead of copiing the values without need.
$lineno = 10;
$vehicleno = 1;
foreach $vehicle ( @{ $config->{'Vehicle'} } ) {
$auctionid = "$vehicle->{AuctionID}";
$make = "$vehicle->{Manufacturer}";
$model = "$vehicle->{Model}";
[...]
Same as above, there is no need to copy the values, it just makes your script much longer than needed.
$image1 = "$vehicle->{Images}->{Image_1}";
$image2 = "$vehicle->{Images}->{Image_2}";
$image3 = "$vehicle->{Images}->{Image_3}";
$image4 = "$vehicle->{Images}->{Image_4}";
$image5 = "$vehicle->{Images}->{Image_5}";
$image6 = "$vehicle->{Images}->{Image_6}";
$image7 = "$vehicle->{Images}->{Image_7}";
$image8 = "$vehicle->{Images}->{Image_8}";
$image9 = "$vehicle->{Images}->{Image_9}";
$image10 = "$vehicle->{Images}->{Image_10}";
$image11 = "$vehicle->{Images}->{Image_11}";
$image12 = "$vehicle->{Images}->{Image_12}";
Besides the previous comment, this is a wonderful loop example. Here is my suggestion:
for my $ImgNo (1..12) {
# Storing them into an array is easy:
$image[$_] = $vehicle->{Images}->{'Image_'.$ImgNo};
# or you could also skip empty values:
push @image,$vehicle->{Images}->{'Image_'.$ImgNo} if $vehicle->{Imag
+es}->{'Image_'.$ImgNo} ne '';
# or you may want to keep the variable names:
eval('$image'.$ImgNo.'=$vehicle->{Images}->{"Image_".$ImgNo};');
}
This makes three lines which were 12 before.
###########Write CSV############
$opath = "e:/out/" ;
$add='0';
$add2='xx.csv';
$add5 = $exportid.$lineno ;
$add3 = $opath.$exportid.$lineno.$add2 ;
$add6 = "xx";
$filename = $exportid.$lineno.$add2;
$mileage =~ tr/,//d ;
$damage =~ tr/,//d ;
$trim =~ tr/,//d ;
open (FILENAME,">$add3");
Just a text issue: I prefer calling it FILEHANLDE, because FILENAME is not what is inside, but it doesn't make any difference to Perl...
print FILENAME "Ref No,Make,Model,Trim,Vehicle Sub Class,Colour,Engine
+ Size,Fuel Type,Transmission,Year,Mileage,Registration No,Keys Suppli
+ed,Damage Report,VAT Applicable,Condition,FSH,ABI Category,Reserve,Ch
+assis No,Vehicle Source,Agent,Region,Vehicle Location,Source Name,Dat
+e Approved\n";
print FILENAME "$add5,$make,$model,$trim,$doors,$colour,$cc,$fueltype,
+$transpeed $trantype,$year,$mileage,$reg,$keyssupplied,$damage,$hasva
+t,$starts $drives,No,$abicat,$reserve,na,$memid,$memid,$memid,$memid,
+$auctionid,na,\n";
This last print could be made much more readable:
print FILENAME join(',',
$exportid.$lineno, #$add5 is defined this way some lines before
$vehicle->{Manufacturer},
$vehicle->{Model},
[...]
@image,
)."\n";
This one uses join and the original names for the fields.
close FILENAME ;
print "<table width=400>";
print "<tr><td>$vehicleno. ";
print "$filename Created </td>";
#############Change IMG Name##############
$pga ="a.jpg";
$pgb ="b.jpg";
$pgc ="c.jpg";
$pgd ="d.jpg";
$pge ="e.jpg";
$pgf ="f.jpg";
$pgg ="g.jpg";
$pgh ="h.jpg";
$pgi ="i.jpg";
$pgj ="j.jpg";
$pgk ="k.jpg";
$pgl ="l.jpg";
if ($image1 ne ""){$jpg1 ="$add5$add6$pga";
rename ($ipath.$image1, $ipath.$jpg1);
print "<td colspan=2>Image 1 Renamed</td></tr>";
}
Here we could you really cool Perl features:
$Char = "a";
for (@images) {
next if $_ eq ''; # if you didn't filter them above
rename $ipath.$_,$ipath.$add5.$add6.($Char++).'.jpg';
}
First, I defined a variable for the current char, the first one is "a".
I assumed that you switched to an array before which is easier to work with. This loop goes through the array and (next line) skips empty values if there is a risk that they may make it into the array.
The next line is using the current loop item $_ and renaming it to $Char.jpg
Like (nearly) any programming language, Perl could count up, but in Perl this also works for letters. The "a" defined before is counted up to a "b" here, but - and this is why the ++ are BEHIND the variablen name - the old value is returned and used in the loop run. Pretty cool, isn't it?
I skiped the print, but I guess that you could handle this yourself...
Let's stop here as I have nothing to say about the rest.
One thing left for reading directories:
opendir THEDIR, "$ipath";
for $file (grep (/$str/, readdir THEDIR)) {
# Do the work
}
closedir THEDIR;
This is shorter and you save the creattion of a temporary array.
Don't worry, I also wrote code like this when I was a Perl beginner. Please don't treat this as critic, I'm very happy that people are still learning Perl! This post should be a review of things you could do to make your code more readable and easier to maintain.