[LA.pm] little help??

[Bob Mathews]

On Sep 28, 2005, at 10:14 AM, FamiLink Admin wrote:
> What I have kinda works but I am not sure if it is the best practice.

First piece of general advice:
    use strict;
    use warnings;
at the top of every program. It looks like your code will not work when 
you first add those lines, but you will build character by making it 
work again.

Second piece of general advice: indent consistently. It will make your 
code much more readable.

>     local($ipb) = @_;

You want "my", not "local". Local is for use with global variables, 
which are bad. I'm guessing you learned from a very old perl book which 
predated "my".

>         open my $slog, "-|", "tail -n 50000 $log" or die "Unable to 
> open $log:$!\n";

This code doesn't work. Try it when $log doesn't exist -- it won't die 
(though you will see an error message from tail).

Paranoia time: are you certain that $log doesn't contain anything 
dangerous that will make the shell misbehave? Probably not, but if 
you're putting this in a CGI script that takes $log as a parameter, 
it's something you need to think about.

>         open (OUTPUT,">/etc/squid/iplist.txt");
>         open (OUTPUT2,">/etc/squid/SuspendIpList.txt");

Using open with a bareword filehandle name like this is the old style. 
It makes global filehandles, and once again, globals are bad. Using 
"open my $foo" like you did in the previous line is the new style. 
Mixing styles makes the code look crufty, but isn't necessarily bad.

Include error detection. At least slap an "or die" on the end of the 
open statement. That way, you'll know what's wrong if your /etc/squid 
directory gets set read-only one day.

Imbedding important pieces of information (like the output file names) 
in the code is considered bad form. Pull those up to the top of the 
program in a constant declaration, is my advice, though other advices 
are possible.

Here's the adjusted version of the code:
    use constant IP_LIST_FILE => "/etc/squid/iplist.txt";
    ...
    sub Scanlog {
      open my $output, ">", IP_LIST_FILE or die "Can't write 
@{[IP_LIST_FILE]}: $!";
      ... print $output "blah blah blah\n"; ...
      close $output;
    }

>        # use an array slice to select the fields we want
>         @data = (split ,$_)[1,4,10,5,7];

It would be a good idea to include, in a comment in your code, an 
example of a properly formatted input line. Future generations of 
program maintainers would also appreciate explanations of the different 
fields, but producing that level of documentation starts to get 
tedious.

Keeping all your data in the @data array makes things hard to 
understand. When I see $data[2] in the code, I have to look at the 
array slice and see that $data[2] corresponds to field 10 from the 
source line, then count fields on the line to see that it means the 
category, PO. It's better to use descriptive variable names, like this:
    my ($time, $ip, $category, $other_ip, $url) = (split " ", 
$_)[1,4,10,5,7];
Yes, I see you used @data in a print statement later on, but weirding 
the entire body of the loop just to make a print statement look nice is 
the Wrong Thing.

>         $hr = (split /:/ ,$data[0])[0];

I prefer this. Matter of taste.
    my ($hr) = split /:/, $time;

>         foreach (/$data[2]/){
>         $matches += 1 ;
>         }

I'm not sure what you wanted this to do, but I'm pretty sure it doesn't 
do it. You're taking the last thing from the line and using it as a 
dynamic regex, implicitly matching against the entire line. That will 
always match (unless $data[2] contains metacharacters (in which case it 
might explode) or $data[2] is empty (in which case something unexpected 
will happen)). Dynamic regexes are slow, error-prone, and generally to 
be avoided if possible. Also, your "foreach" loop is really behaving 
like an "if".

>         print OUTPUT "$matches,", "$hour, ","$ip1, ", "@data","\n";

Why the extra quotes and commas?
    print OUTPUT "$matches,$hour,$ip/32,$category,$other_ip,$url\n";

>         $matched = $matches;

$matched and $matches are globals, and globals are bad. What I tell you 
three times is true.

> }}}}

Yuck. Most people put each closing bracket on its own line, so they can 
see where it matches up.

  -bob