From: RichT mailto:[EMAIL PROTECTED] wrote:
: im still very new to perl and, iv got my script
: working,
It doesn't work for me. What are you using on the
command line to make it work? According to the usage shown
below, this script should fail every time.
: what is does in searches through a large data file finds the
: segments i need and outputs the data in a more use-full
: format. As i said id does work but im
: no to convinced the structure is very clean
: so i thort maybe some one could give me some pointers.
: yours
: most gratefully RichT
First, I am going to piss off you or someone else on the
list. That's not my intent. It's is my experience. My intent is
to illustrate the errors in this script and the errors and
inconsistencies in your programming style. It, in no way is
meant to ridicule you or condemn your script.
Second, some corrections are stylistic. For example, the
one space indents for some foreach blocks don't make sense
with all other indents being four spaces. This is your style.
There nothing wrong with that (other than being
inconsistent). Your style will not always match mine.
Third, some errors are not errors. Some idioms are
more accepted than others. Not using strict or warnings on
a script sent to a mailing list is not really an error, but
it is a very bad idea.
Fourth, I am going to spend more time on the minuses
than on the plusses. Live with it.
Fifth, I'm not a big fan of mixed case variable names.
Any multiple word variables I add will use underscores. I
also hate parenthesies. I will quietly change them in
rewrites.
Last, my opinion is just that: my opinion. It is not
fact and you do not have to follow my advice. Some of it may
even be poor advice. Shop around and ask questions.
: scanPoller.cfg.pl==
:
: #!/usr/local/bin/perl
:
: use strict;
: use warnings;
Great start.
: use Getopt::Long;
:
: my($inFile,$USAGE,$showKey,$site,$found,$foundKeys,
:@dataFile,@foundSegments,$value);
: my($opt_inFile, $opt_showAll, $opt_help ) = (0,0,0);
This is perl. It is not VB or C. Stop declaring your
variables at the top of your code block. It WILL bite you
in the ass. The rest of my comments will assume these were
not declared here.
Why is $USAGE in all caps?
: my $feildName = agentAddress;
: my $showFields = segment,agentAddress,community;
Line up those assignments and don't use double quotes
unless you're interpolating variables.
my $fieldname = 'agentAddress';
my $showFields = 'segment,agentAddress,community';
: $USAGE = USAGE_TEXT;
my $usage = USAGE_TEXT;
[snipped info due to wrapping problems]
: USAGE_TEXT
: GetOptions ( inFile=s,
: findFeild=s = \$feildName,
: showFeilds=s = \$showFields,
: showAll,
: help|h|H|?|HELP
: );
Field is spelled wrong. Get an editor that has a spell
checker.
Line things up and use single quotes over double quotes.
When I see double quotes, I assume you have data in there to
interpolate and I look for it. This slows me down. I assume
it will slow down another script maintainer as well.
We need to use global variables to use the above. Let's
avoid them here.
GetOptions (
'inFile=s', = \$inFile,
'findField=s' = \$fieldName,
'showFields=s' = \$showFields,
'showAll' = \$opt_showAll,
'help|h|H|?|HELP' = \$opt_help,
);
: if ($opt_help == 1) {print $USAGE; exit; } # print out
# help and
# exit
Using trailing comments sparingly. Don't bunch up if
statements. Either use a statement modifier.
die $usage if $opt_help;
Or spell it out.
if ( $opt_help ) {
print $usage;
exit;
}
On my system (Windows XP) this doesn't work. $opt_help
always remains 0. This works.
use strict;
use warnings;
use Getopt::Long 'GetOptions';
my( $inFile, $showAll, $fieldName, $showFields );
{
$fieldName = 'agentAddress';
$showFields = 'segment,agentAddress,community';
my $help
GetOptions(
'inFile=s', = \$inFile,
'findField=s' = \$fieldName,
'showFields=s' = \$showFields,
'showAll' = \$showAll,
'help|h|H|?|HELP' = \$help,
);
die usage() if $help;
}
sub usage {
return USAGE_TEXT;
usage: $0 [-inFile input filename] [-findField fieldName (default is
agentAddress)]
[-showFields fields to output (default is
segment,agentAddress,community)] [-showAll show all fields]
[-help this help text]
USAGE_TEXT
}
__END__
: # start finding results
: if ($inFile){ # find results if we have in -inFile
Is the comment really needed? if ( $inFile )
pretty much says the same thing.
Note that in the original script $inFile is never set
to a value. This code block is never executed. Perhaps
we can see the stupidity in