in reply to Script Help!

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 ;
      }

[download]

can be written in a single line:

      $userhash{$username}++;

[download]

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 ...

[download]

would become

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

[download]

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

Replies are listed 'Best First'.
Re^2: Script Help!
by raybies (Chaplain) on Nov 03, 2010 at 12:23 UTC

    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" ;

    [download]

    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

    [download]

    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

[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom