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 declare all variables at the
top. If this variable were set when it was first used,
we would receive an error and know we had never set it to
begin with.
: open DFILE, "$opt_inFile" # open $inFile and
: # read in or die
Is the comment really needed? The 'open' function
opens files and the "or die" is right down there. Don't
quote variables unnecessarily. "$opt_inFile" is not
always the same as $opt_inFile. Even in an open
statement it looks suspect.
: or die " could not open $opt_inFile";
Sentences start with capital letters, not spaces.
Perl provides a nice error in the $! variable. The
qq() function is discussed in 'perlop'.
or die qq(Cannot open "$opt_inFile": $!);
So we have a flag named $inFile, a file handle
named DFILE, an option named $opt_inFile, and an array
named @dataFile. Doesn't seem too consistent to me.
: @dataFile = <DFILE>;
You're chomping all the values below.
chomp( @dataFile = <DFILE>;
: close DFILE;
:
: foreach $value (@dataFile) { # loop for each
: # line/site in
: # dataFile
Why is $value a file scoped variable. It is only
used in this loop.
foreach my $value ( @dataFile ) {
: chomp $value ;
What's the space ("$value ;") for?
: @foundSegments=findVars($feildName,$value);
White space, white space, white space!
@foundSegments = findVars( $fieldName, $value );
: }
This whole loop is the same as this statement.
@foundSegments = findVars( $fieldName, $dataFile[-1] );
: } elsif ($ARGV[0]) { # read in value from comandline
This does not match the usage. $ARGV[0] cannot be
a value unless the usage statement is wrong. When
GetOptions() is done, @ARGV should be empty. If it is
not, the usage statement is wrong.
: foreach $value ($ARGV[0]) { # loop for each
: # line/site in
: # dataFile
$ARGV[0] is a scalar value. This will loop only one
time.
: @foundSegments=findVars($feildName,$value);
: }
This loop is the same as this.
@foundSegments = findVars( $feildName, $ARGV[0] );
: } else {print $USAGE; exit; } #quit if no values are
: supplyed...
The comment is probably not helping. See above for comments
on this rewrite.
} else {
print $USAGE;
exit;
}
So far we have the following logic.
if ( $inFile ) {
# will always fail
} elsif ( $ARGV[0] ) {
# should always fail
} else {
print $USAGE;
exit;
}
Which is the same as
print $usage;
exit;
I'm going to stop here. You need to show how this
script works and perhaps show my error in interpreting
the usage statement. An example input file would also
help testing.
HTH,
Charles K. Clarkson
--
Mobile Homes Specialist
254 968-8328
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
<http://learn.perl.org/> <http://learn.perl.org/first-response>
