blackice69 has asked for the wisdom of the Perl Monks concerning the following question:

Hello.. I asked a question here earlier about making a logfile and reportfile command line argument type script. Here is what I have.

#!C:\Perl\Bin\Perl.exe # Enable Pragmatic Modules use warnings ; use strict; # Define Variables and Hashes my $numelements ; my $done = 3 ; my $count = 0 ; my $username = "" ; my %userhash ; my $inputfile = "" ; my $outputfile = "" ; my $userlist = "" ; my $record = "" ; my $bigcounter = 0 ; # Argument testing $numelements = @ARGV ; print "\n$numelements\n" ; # Check for 0 arguments and switches, apply error message if true if ( $numelements == 0 ) { print "\n *** Error : Command-line switches and arguments must be p +rovided. ***\n\n" ; print " $0 -l <log_file> -u <userlist_file> -r <report_file>\n\n" ; print " -l <log_file> The relative or absolute path to +the webserver logfile.\n" ; print " -u <userlist_file> The relative or absolute path to +the user list file.\n" ; print " -r <report_file> The relative or absolute path to +the generated report file.\n\n" ; exit 200 ; } # Check to osee that they gave more or less than 6 arguments, appply a +pproproiate error message. if (( $numelements > 0 ) and ( $numelements !~ 6 )) { print "\n *** Error : Too many (or too few) command-line switches a +nd arguments were provided. ***\n\n" ; print " $0 -l <log_file> -u <userlist_file> -r <report_file>\n\n" ; print " -l <log_file> The relative or absolute path to +the webserver logfile.\n" ; print " -u <userlist_file> The relative or absolute path to +the user list file.\n" ; print " -r <report_file> The relative or absolute path to +the generated report file.\n\n" ; exit 201 ; } # Check for double switches if (( $ARGV[0] =~ $ARGV[2] ) or ( $ARGV[2] =~ $ARGV[4] ) or ( $ARGV[0] =~ $ARGV[4] )) { print "\n *** Error : Incomplete (or incorrect) command-line sw +itches and arguments were provided. ***\n\n" ; print " $0 -l <log_file> -u <userlist_file> -r <report_file>\n\ +n" ; print " -l <log_file> The relative or absolute path + to the webserver logfile.\n" ; print " -u <userlist_file> The relative or absolute path + to the user list file.\n" ; print " -r <report_file> The relative or absolute path + to the generated report file.\n\n" ; exit 202 ; } # Start a loop to check for all correct switches, and apply input file +names to variables while ( $done != 0 ) { if ( $count == 6 ) { print "\n *** Error : Incomplete (or incorrect) command-line swit +ches and arguments were provided. ***\n\n" ; print " $0 -l <log_file> -u <userlist_file> -r <report_file>\n\n" + ; print " -l <log_file> The relative or absolute path t +o the webserver logfile.\n" ; print " -u <userlist_file> The relative or absolute path t +o the user list file.\n" ; print " -r <report_file> The relative or absolute path t +o the generated report file.\n\n" ; exit 202 ; } if ( $ARGV[$count] =~ /^-l$/i ) { $inputfile = $ARGV[$count + 1] ; $done-- ; } if ( $ARGV[$count] =~ /^-u$/i ) { $userlist = $ARGV[$count + 1] ; $done-- ; } if ( $ARGV[$count] =~ /^-r$/i ) { $outputfile = $ARGV[$count + 1] ; $done-- ; } $count = $count + 2 ; } # Check for input file existance and begin to parse file, otherwise di +splay appropriate error message if ( -e $inputfile ) { open( INPUTDATAFILE, "< $inputfile" ) or die " \n\nError: $!\n\n" ; while ( defined( $record = <INPUTDATAFILE> ) ) { if ( $bigcounter == 100000 ) { print "." ; $bigcounter = 0 ; } chomp( $record ) ; if($record =~ /\/\~(c(s|e)[0-9][0-9][0-9][0-9])/) { $username = $1 ; if(exists $userhash{$username}) { $userhash{$username}++; } else{ $userhash{$username}=1 ; } } $bigcounter++ ; } $numelements = %userhash ; print "$numelements\n" ; }else { print "\n *** Error : Webserver logfile '$inputfile' : NOT FOUND. * +**\n\n" ; exit 100 ; } # Check for userlist file, or display apropriate error message if ( -e $userlist ) { }else { print "\n *** User list file '$userlist' : NOT FOUND. ***\n\n" ; exit 101 ; } system "( Pause ) " ;

Now.. My problem is.. I want to find a different way to do the following:

# Start a loop to check for all correct switches, and apply input fil +enames to variables while ( $done != 0 ) { if ( $count == 6 ) { print "\n *** Error : Incomplete (or incorrect) command-line swit +ches and arguments were provided. ***\n\n" ; print " $0 -l <log_file> -u <userlist_file> -r <report_file>\n\n" + ; print " -l <log_file> The relative or absolute path t +o the webserver logfile.\n" ; print " -u <userlist_file> The relative or absolute path t +o the user list file.\n" ; print " -r <report_file> The relative or absolute path t +o the generated report file.\n\n" ; exit 202 ; } if ( $ARGV[$count] =~ /^-l$/i ) { $inputfile = $ARGV[$count + 1] ; $done-- ; } if ( $ARGV[$count] =~ /^-u$/i ) { $userlist = $ARGV[$count + 1] ; $done-- ; } if ( $ARGV[$count] =~ /^-r$/i ) { $outputfile = $ARGV[$count + 1] ; $done-- ; } $count = $count + 2 ; }
How Would I accomplish this? I am being told this is an odd way to do this.

Replies are listed 'Best First'.
Re: Script Help!
by kcott (Archbishop) on Nov 02, 2010 at 17:33 UTC

    Take a look at Getopt::Long. It's part of the standard Perl distribution (no CPAN installations required) and the documentation has lots of tips and examples.

    As a general rule, when you find yourself writing the the same (or very nearly the same) code over and over again, think subroutine. Most of those blocks of print statements are identical except for the error message: it would have been better to put that in a subroutine (perhaps called usage) and then replaced all those blocks of code with usage($error_message)

    The doco shows an alternative method using POD and a GetOptions function which should help with checking the commandline options.

    -- Ken

Re: Script Help!
by Utilitarian (Vicar) on Nov 02, 2010 at 17:18 UTC
    Sometimes conciseness can be good ;)
    If I were in your situation I'd use Getopt::Std

    It will have dealt with issues you haven't considered. You could also consider setting defaults for the various options to use if none is set.

    print "Good ",qw(night morning afternoon evening)[(localtime)[2]/6]," fellow monks."
Re: Script Help!
by kcott (Archbishop) on Nov 03, 2010 at 06:59 UTC

    A few more pointers on your code.

    You don't need to check for existence when incrementing the value of a hash key:

    if(exists $userhash{$username}) { $userhash{$username}++; } else{ $userhash{$username}=1 ; }

    can be written in a single line:

    $userhash{$username}++;

    It's more readable and maintainable to use Quantifiers (see perlre) in a regular expression than using a string of repeated characters or character classes:

    So, [0-9][0-9][0-9][0-9] would have been better if written like [0-9]{4} (in that specific instance, \d{4} would have been even better).

    When checking for a value that's being incremented or decremented, it's good defensive programming to check not just the terminating value but also all values beyond that. This will often avoid infinite loops.

    So, $bigcounter == 100000 would be better as $bigcounter >= 100000 and similarly for $done != 0.

    When using open, it's good practice to separate the mode (e.g. '<' for reading, '>>' for appending, etc.) from the file name. It's also good practice to use a lexically scoped filehandle (e.g. my variable) rather than a global. Also, when using or die ... the parentheses are not required - they are if you use || die ....

    Putting that all together

    open( INPUTDATAFILE, "< $inputfile" ) or die ...

    would become

    open my $inputdatafile, '<', $inputfile or die ...

    In closing, I will compliment you on your code in general (neatly laid out, meaningful variable names and so on) - don't stop doing that. :-)

    -- Ken

      also, while we're at it, whenever you find yourself doing a whole bunch of print statements I like to use what's called a unix 'here document'. (google it).

      So that something like this:

      print " $0 -l <log_file> -u <userlist_file> -r <report_file>\n\n" +; print " -l <log_file> The relative or absolute path t +o the webserver logfile.\n" ; print " -u <userlist_file> The relative or absolute path t +o the user list file.\n" ; print " -r <report_file> The relative or absolute path t +o the generated report file.\n\n" ;

      looks like this:

      print <<EOUsage; $0 -l <log_file> -u <userlist_file> -r <report_file> -l <log_file> The relative or absolute path to the webse +rver logfile. -u <userlist_file> The relative or absolute path to the user +list file. -r <report_file> The relative or absolute path to the gener +ated report file. EOUsage

      You don't have to worry about sticking in '\n's wherever and no need to backslash protect a lot of special characters, you can stick your variables in it, and you can see what your output will look like because it is exactly as you typed it... like magic.

      Ain't perl cool? :) Good luck, --Ray

Re: Script Help!
by eff_i_g (Curate) on Nov 02, 2010 at 19:19 UTC