PERL Beginners - Code check for my program

Author

Code check for my program

Shanna

2006-07-26, 6:57 pm

Hello,

I am wondering if someone could please take a look at this code to make
sure I am on the right track. I am teaching perl to myself and would
just like to make sure I am on the right path with what I have so far.
My code is suppose to take an input file check a one field to make sure
it is populated, then if it is check to make sure the required fields
for the given action are populated. If everything is correct then it
is to write the record to an Excel file (which I don't have the code
for this yet as I am trying to find how to do this).

Here is my code as it sits now:

#!/usr/bin/perl

use strict;
use warnings;

my $vendorFile = $ARGV[0];

open(my $VendorFile, $InFile) || die "Can't open file '$inFile': $!";

my @correctColumnName = (
"Key_Number", # 0
"Ferguson_Code", # 1
"Code_Change", # 2
"Manufacturer_Code", # 3
"Action", # 4
"Block_ID", # 5
"Vendor", # 6
"Block_Header", # 7
"Block_SubHeader", # 8
"B2B_Description", # 9
"Block_Image_File_Name", #10
"Line_Item_Description", # 11
"Installation_Instructions_PDF_File_Name", # 12
"Product_Specifications_PDF_File_Name", # 13
"Parts_Breakdown_PDF_File_Name", # 14
"Product_Image_File_Name" # 15
"Warranty_PDF" #16
"Model_Name" #17
"Size" #18
"Attribute_1" #19
"Attribute_2" #20
"Attribute_3" #21
"Attribute_4" #22
"Attribute_5" #23
"Required_Accessories" #24
"Optional_Accessories" #25
"
);

while (<VENDORFILE> ) {
chomp;
my @linedata = split '\t';
my ($fieldname, $fieldPosition);
my $i;

foreach $i (0 .. $#linedata) {
$linedata[$i] = trimString($lineData[$i]);
}
if ($. == 1) {
my @columnName = split '\t';
my $i = 0;

foreach $i (0 .. $#correctColumnName) {
if ($correctColumnName[$i] ne $columnName[$i] {
die "Error: Columns are out of order
\n\t($columnName[$i] should be
$correctColumnName[$i]\n\t"; }
else {
$columnPosition{$correctColumnName[$i]} = $i; }
}
} else {
$lineCount++;
if (defined($linedata[6]) && $linedata[6] =~ /\w/) {
my $vendorname = $linedata[6];
$vendorname =~ tr/[a-z]/[A-Z]/;
if (!defined($vendorxlations{$vendorname}))
{
print "Error on line $.: Vendor Name $linedata[6] not found in
Vendorinput.txt\n":
$errorcount++;
}
}
if (!defined($linedata[4] || !$linedata[4] =~ /\w/) {
print "Error on line $.: No action in the Action column\n";
} elsif ($linedata[4] eq "NEWPRODUCT") {
foreach $fieldname (@correctColumnName[0, 1, 4, 6]) {
elsif ($linedata[4] eq "DEACTIVEATEPROD" || "DELETEPROD") {
foreach $fieldname (@correctColumnName[2])}

Thank you for any comments you provide! Have a wonderful day!
Shanna

Paul Lalli

2006-07-26, 6:57 pm

Shanna wrote:
> I am wondering if someone could please take a look at this code to make
> sure I am on the right track.

Shanna,

I can't parse through the whole code at the moment, so I'll simply
point out some danger-zones that jump out at me...

> #!/usr/bin/perl
>
> use strict;
> use warnings;

GOOD!!!

> my $vendorFile = $ARGV[0];

What if the user doesn't provide a file to the program? Traditionally,
one writes something like:
my $vendorFile = shift or die "Usage: $0 <filename>\n";

> open(my $VendorFile, $InFile) || die "Can't open file '$inFile': $!";

Where did $InFile come from?
You now have a filename called $vendorFile and a filehandle called
$VendorFile. That's *begging* for a bug to bite you.
You are using the two argument form of open. This is generally
considered bad form...

open my $vendorFH, '<', $vendorFile or die "Can't open file
'$vendorFile': $!";

> my @correctColumnName = (
> "Key_Number", # 0
<snip>
> "Optional_Accessories" #25
> "

Syntax error?

> );
>
>
> while (<VENDORFILE> ) {

Where did VENDORFILE come from?

while (<$vendorFH> ) {

> chomp;
> my @linedata = split '\t';
> my ($fieldname, $fieldPosition);
> my $i;

Declare your variables in the shortest scope possible.

>
> foreach $i (0 .. $#linedata) {
> $linedata[$i] = trimString($lineData[$i]);

where did @lineData come from?

These are the kinds of things use strict protects you from. Please at
least fix your syntax errors before posting to a forum like this.

> }

foreach my $elem (@linedata) {
$elem = trimString($elem);
}

(that assumes, of course, that trimString really does return the
trimmed version of its input....)

> if ($. == 1) {
> my @columnName = split '\t';
> my $i = 0;

You're redeclaring a variable you're already using, and again not in
the shortest scope possible...

>
> foreach $i (0 .. $#correctColumnName) {

foreach my $i (0..$#correctColumnName) {

> if ($correctColumnName[$i] ne $columnName[$i] {
> die "Error: Columns are out of order
> \n\t($columnName[$i] should be
> $correctColumnName[$i]\n\t"; }
> else {
> $columnPosition{$correctColumnName[$i]} = $i; }
> }
> } else {
> $lineCount++;
> if (defined($linedata[6]) && $linedata[6] =~ /\w/) {
> my $vendorname = $linedata[6];
> $vendorname =~ tr/[a-z]/[A-Z]/;

You are confusing the tr/// semantics with the s/// semantics. tr///
takes simply a list of characters. It does not use character classes.
This particular line will not do any harm, because you are litterally
telling tr to replace all [ with [ and all ] with ], which are both
no-ops.

> if (!defined($vendorxlations{$vendorname}))
{
> print "Error on line $.: Vendor Name $linedata[6] not found in
> Vendorinput.txt\n":
> $errorcount++;
> }
> }
> if (!defined($linedata[4] || !$linedata[4] =~ /\w/) {
> print "Error on line $.: No action in the Action column\n";
> } elsif ($linedata[4] eq "NEWPRODUCT") {
> foreach $fieldname (@correctColumnName[0, 1, 4, 6]) {

You seem to have forgotten the block to this foreach loop.

> elsif ($linedata[4] eq "DEACTIVEATEPROD" || "DELETEPROD") {

This statement will always be true. The || has a lower precedence than
the eq, so you're asking if $lindata[4] is equal to 'DEACTIVEATEPROD'
(and btw, is that spelled right?) or if the string 'DELETEPROD' is
true. Since 'DELETEPROD' is not any of 0, "0", "", or undef, it is
true.

You probably meant:
elsif ($linedata[4] eq 'DEACTIVATEPROD' or $linedata[4] eq
'DELETEPROD') {

> foreach $fieldname (@correctColumnName[2])}

And you forgot the block for this foreach as well.

Hope that helps,
Paul Lalli

Shanna

2006-07-26, 6:57 pm

Paul Lalli wrote:
> Shanna wrote:
>
> Shanna,
>
> I can't parse through the whole code at the moment, so I'll simply
> point out some danger-zones that jump out at me...
>
>
> GOOD!!!
>
>
> What if the user doesn't provide a file to the program? Traditionally,
> one writes something like:
> my $vendorFile = shift or die "Usage: $0 <filename>\n";
>
>
> Where did $InFile come from?
> You now have a filename called $vendorFile and a filehandle called
> $VendorFile. That's *begging* for a bug to bite you.
> You are using the two argument form of open. This is generally
> considered bad form...
>
> open my $vendorFH, '<', $vendorFile or die "Can't open file
> '$vendorFile': $!";
>
> <snip>
>
> Syntax error?
>
>
> Where did VENDORFILE come from?
>
> while (<$vendorFH> ) {
>
>
> Declare your variables in the shortest scope possible.

Forgive ignorance here, but what do you mean by shortest scope
possible?
>
>
> where did @lineData come from?
>
> These are the kinds of things use strict protects you from. Please at
> least fix your syntax errors before posting to a forum like this.
>
>
> foreach my $elem (@linedata) {
> $elem = trimString($elem);
> }
>
> (that assumes, of course, that trimString really does return the
> trimmed version of its input....)
>
>
> You're redeclaring a variable you're already using, and again not in
> the shortest scope possible...
>
>
> foreach my $i (0..$#correctColumnName) {
>
>
> You are confusing the tr/// semantics with the s/// semantics. tr///
> takes simply a list of characters. It does not use character classes.
> This particular line will not do any harm, because you are litterally
> telling tr to replace all [ with [ and all ] with ], which are both
> no-ops.

I think I am cofused. I thought that line was to make the word either
upper or lower case.

>
>
>
> You seem to have forgotten the block to this foreach loop.
>
>
> This statement will always be true. The || has a lower precedence than
> the eq, so you're asking if $lindata[4] is equal to 'DEACTIVEATEPROD'
> (and btw, is that spelled right?) or if the string 'DELETEPROD' is

Yeah I spelled it wrong, caught that as I was reviewing my code after I
made this post.

> true. Since 'DELETEPROD' is not any of 0, "0", "", or undef, it is
> true.
>
The || symbol is an or function though isn't it?

> You probably meant:
> elsif ($linedata[4] eq 'DEACTIVATEPROD' or $linedata[4] eq
> 'DELETEPROD') {
>
>
> And you forgot the block for this foreach as well.
>
>
> Hope that helps,
> Paul Lalli

Thanks Paul as always your a big help and do a very good job at
explaining errors, I'm learning alot from your pointers.

Paul Lalli

2006-07-26, 6:57 pm

Shanna, I very much appreciate the effor you put into your replies,
especially interspercing your responses with my quoted text. However,
please make an effort to trim the quoted material down to the relevant
bits, as I have done below...

Shanna wrote:
> Paul Lalli wrote:
>
> Forgive ignorance here, but what do you mean by shortest scope
> possible?

It basically means to declare your variable the first time you use it,
and not before. In this case, after your for loop ends, $i still
exists. That can lead to problems later on if you attempt to use $i
accidentally. If you declared it at the foreach loop itself, Perl
would have prevented you (via use strict) from accidentally using it
after it has gone out of scope.

This policy also prevents you from needing to search 100s of lines of
code to find everything that has happened to a variable between the
time it was declared and your current bug. If you limit it to only the
smallest scope (ie, usually the inner-most { } block necessary), you
have a much shorter amount of code to look at to find your bug.
[color=darkred]
>
> I think I am cofused. I thought that line was to make the word either
> upper or lower case.

That line will very specifically make all lowercase characters in
$vendorname into uppercasecharacters. I was merely pointing out that
neither set of the [ and ] should be there. It should be:
$vendorname =~ tr/a-z/A-Z/;

>
> Yeah I spelled it wrong, caught that as I was reviewing my code after I
> made this post.
>
> The || symbol is an or function though isn't it?

Yes it is. But you're missing the point. Your line parsed as:
($linedata[4] eq "DEACTIVEATEPROD") || "DELETEPROD"
whereas you meant, in english, "$linedata[4] is equal to either
'DEACTIVATEPROD' or 'DELETEPROD'". Perl expressions don't work that
way. You have to make both tests. That is, you have to say "either
$linedata[4] is equal to 'DEACTIVATEPROD' or $linedata[4] is equal to
'DELETEPROD'".

The fact that I changed I changed your || to or is a side-issue, and
didn't relate to your bug. Sorry for that confusion. I just usually
default to using the lower-precendence 'or' unless I for some reason
need the higher-precedence of '||'

> Thanks Paul as always your a big help and do a very good job at
> explaining errors, I'm learning alot from your pointers.

Glad I could help.

Paul Lalli

Sponsored Links