Re: RFC on script // GetOptions problems

2004-10-05 Thread RichT
Fist of all thank you both very much, this has been a good learning experience.

[snip]
 From: RichT mailto:[EMAIL PROTECTED] wrote:
 
 :  im still very new to perl 
 
 : 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.

good stuff. =)
 
 
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.


i use them because the software i have been using for 
nearly 4 years does, it habit now. (and it keeps things to a standard)

[snip]
 The above is ok, but you might consider taking the advice of the
 Getopt::Long docs and using Pod::Usage to generate error messages and
 help text via pod.
still not looked at this, it on my list tho


oki  have done some work... i havent done every thing suggested i
started to get lost
but i think its much better looking now ...

My main problem now is that if i have a long inFile sub findVars 
has to be processed a lot of times which takes a long time (as
poller.cfg can have over 30,000 records) what can i do to increase the
speed.

i would also like to add some checks ie for a valid IP address should
i do this with some regX or is there a module i can use?



scanPoller.cfg.pl==
more scanPoller.cfg.pl
#!/usr/local/bin/perl
#
use strict;
use warnings;
use Getopt::Long;

Getopt::Long::config qw(no_ignore_case);


my ( $inputFile, $outputAllFields, $searchKey, $outputFields );
{

$searchKey = 'agentAddress';
$outputFields = 'segment,agentAddress,community';

my $needHelp;
GetOptions(   'inFile=s'  = \$inputFile,
  'findField=s'   = \$searchKey,
  'showFields=s'  = \$outputFields,
  'listAllFields' = \$outputAllFields,
  'help|h|H|?|HELP'   = \$needHelp
  );
die usage() if $needHelp;
}


# Data collection
#  if we using an input file
#  else if we have found some values on the cl
#  else quit


my @foundSegments;
if ($inputFile){
open INFILE, $inputFile
  or die qq(Could not open $inputFile: $!);

while (INFILE) {
  chomp;
  push @foundSegments, findVars( $searchKey, $_);
}
close INFILE;

} elsif ($ARGV[0]) {
push @foundSegments, findVars( $searchKey, $ARGV[0]);

} else {
while (STDIN) {
  chomp;
  push @foundSegments, findVars( $searchKey, $_);
}

}


# Data output
#   if request for keys print all keys
#   else print results


if ($outputAllFields) {
foreach ( @foundSegments ) {
  foreach (keys %$_) {
print $_, ;
  }
print \n;
}

} else {
for my $found ( @foundSegments ) {
  foreach my $showKey (split /,/, $outputFields) {
print $found-{$showKey},;
  }
print \n;
}

}


sub usage {
return USAGETEXT;
usage: $0 search value
  if no search value is given input will be taken from std in
  the following options are also availble
  [-inFile filename ] input filename
  [-findField fieldName ] this is the search key to be used
(default is agentAddress)
  [-showFields field names ] feilds to output (default is
segment,agentAddress,community)
  [-listAllFields ] list avalible fields
  [-help] this help text
USAGETEXT
}

sub findVars {

# find infomation form Concord's poller.cfg
# usage:
#  findVars(key to search in,value to search for)
#
# output:
#  an aray of Hashes containing all matched segments and keys



my($findKey, $findValue, $segmentFieldKey, $segmentFieldValue,
%segmentFields, $nullVar, @foundSegments);

# read in Search Key and Value  from parent NOTE make a check for this
$findKey=$_[0] || die qq(Missing Args $findKey $!);
$findValue=$_[1] || die Missing Args $findValue $!;
chomp $findValue;
chomp $findKey;
#my $NH_HOME= $ENV[NH_HOME];  # point to the poller CFG file
my $NH_HOME= .;  # point to the poller CFG file NOTE this is temp
for testing use above line in live


local $/ = }\n;   # set delimiter


open(POLLER, $NH_HOME/poller.cfg) || die can not open : $!;

#s/universalPollList \{//g;

while(POLLER) {
  next unless /^\s+segment/;
  

RE: RFC on script

2004-10-01 Thread Charles K. Clarkson
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