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/; s/\n\s+\}\n//g; s/["{]//g; foreach (split(/\n/)) { ($nullVar,$segmentFieldKey,$segmentFieldValue) = split(/\s+/,$_,3); $segmentFields{ $segmentFieldKey } = $segmentFieldValue ; } if ( $segmentFields{$findKey} eq $findValue ) { push @foundSegments, {%segmentFields } ; } undef %segmentFields; my %segmentFields; } close POLLER; return (@foundSegments); # return the IP and comunity string to main ruteen }; /scanPoller.cfg.pl================================================== exampleExtract form poller.cfg================================== segment "customer-site-Bend" { agentAddress "x.x.x.x" uniqueDeviceId "dfofdkskhjkldsf" mibTranslationFile "cisco-frameRelay-cir.mtf" index "2" index2 "400" deviceSpeed "64000.0" deviceSpeed2 "64000.0" discoverMtf "cisco-frameRelay-cir.mtf" index3 "7" community "public" sysDescr "Cisco Internetwork Operating System Software IOS (tm) C1700 Software (C1700-Y-M), Version 12.1 blar blar..." sysName "routerName" sysLoc "siteLocation" ifDescr "Serial0" ifType "frame-relay" aliasName "PVCreffNumber" nmsKey "routerName Serial0 400" enterpriseId "9" possibleLatencySources "concord, ciscoPing" fullDuplex "1" mediaSpeed "64000.0" mediaSpeed1 "64000.0" statistics "1" } /exampleExtract form poller.cfg================================== example inFile================================== x.x.x.x y.y.y.y z.z.z.z /example inFile================================== > 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 > > -- -- Fnord... http://info-x.co.uk http://23.me.uk -- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] <http://learn.perl.org/> <http://learn.perl.org/first-response>
