in reply to Peruse my code...

Two things stand out from a first pass look.

$message = $message . "somemore"; is better (read: shorter, clearer) written as $message .= 'somemore';. Also, note, you save a little in compile time (load time) if you use 'text', rather than "text" for string constants as the compiler knows it doesn't have to check for things to interpolate.

++ for reporting all errors in one pass, not each one individually as so many damn sites do.

I'm not quite sure why your sub PrintError has both exit 0; and return 1;. The latter will never be seen, and the former means that the script will abort after printing the error text, which maybe what you want, as otherwise the script would go on to say 'Blah Blah, All done'.$/;?

I don't find it aesthetically pleasing to exit a program from within a sub that way. I doesn't make too much difference in a script this short, but as the script grows in size, its likely to become confusing to those poor souls that come along after to maintain your work.

I think I would write that as 

if ($found_err) {
  &PrintError; 
  exit -1; # 0 often means ok, whereas this is an error exit.
}

[download]

And remove the exit 0; from PrintError ... also the if your never going to check its value or if its always going to be the same, the return 1; serves little purpose.

Finally (yes, I know that's more than two things:), by putting the } on the end of the last line of the if block makes it harder to add in statements. Putting it on the next line by itself, it can replace one of your blank lines, retains the value of the white space, clearly indicates where the block ends, and makes it easier to add statements in there. That's all a matter of taste, and often raises near religious ferver, so I'll add, IM(NS)HO! :)

Well It's better than the Abottoire, but Yorkshire!

Replies are listed 'Best First'.
Re: Re: Peruse my code...
by Anonymous Monk on Aug 31, 2002 at 13:07 UTC
    cheers guys. that was most insightfull. All in all, it looks like only a few little things have to be done now. But, i am still stuck on what to do with the files once they have been wrote:

    I have tried but failed to create a script to descend into directory /var/log/accounts/ where there will be several hundred files (randomly named), pickup one of those files, open it and read the contents into variables. The file is arranged like the following 'username,domain,email,service'. Once it has read the contents in and run the code that i have created, it will then need to close that file, delete that file, and go round in a loop until no more files in that directory exist.

    I have asked before, but i am realy stuck and about to smack my head agains my monitor!
[reply]

      I have tried but failed to create a script to descend into directory /var/log/accounts/ where there will be several hundred files (randomly named), pickup one of those files, open it and read the contents into variables. The file is arranged like the following 'username,domain,email,service'. Once it has read the contents in and run the code that i have created, it will then need to close that file, delete that file, and go round in a loop until no more files in that directory exist.

      Ok, ap3k, lets look at what you said. 

      #    ... descend into directory /var/log/accounts/...

my $directory = '/var/log/accounts/';
opendir DIR, $directory or die $!.$/;
#    ... several hundred files ...
my @files = readdir(DIR);
closedir DIR or warn $!.$/;

    ... go round in a loop until no more files ...

for my $file  (@files) {
    # ... open it ...
    open FILE, $file or die $!.$/;                    
    # ... into variables ... username,domain,email,service ...
    my ( $username, $domain, $email, $service ) =         
    split /,/, <FILE>;    # ... read the contents ...
    # run the code that i have created
    #your code here!
    
    # ... close that file, ...
    close FILE or warn $!.$/;
      # ... delete that file ...
    unlink $file;
}
=cut

      [download]

      That's (deliberatly) not perfect code. You will have to read the docs on readdir, split, open, unlink and probably grep, to get it to do something properly, but that will be a relief from abusing that poor monitor with your head!

      Well It's better than the Abottoire, but Yorkshire!
[reply]
[d/l]
      This is what i have come up with so far, but it doesn't work. Any suggestions? 
      
#!/usr/bin/perl

@file = `ls /var/log/accounts`;
foreach $file {
  open(FILE, "<$file") || die;
  @account=<FILE>;
  close(FILE);
  foreach $account {
    ($username, $domain, $email, $service) = split(/,/, $account);
    #
    # Insert rest of my code here
    #
  }
  unlink $file;
}

      [download]
[reply]
[d/l]
        Try typing ls /var/log/accounts into the command shell. You need to change this line:

        
  open(FILE, "</var/log/accounts/$file") || die;

        [download]

        I make the same mistake all the time.

        ____________________
        Jeremy
        I didn't believe in evil until I dated it.

[reply]
[d/l]

In Section Seekers of Perl Wisdom