First Perl/CGI need critique on current code before further developement

[kazack]

This is part of an elaborate web counter that I am in the process of
writing. I am hoping that once I am done with this little project I will be
able to move on to something else. I was told that this is a waste of time
and why am I trying to re-invent the wheel, but by re inventing the wheel
you learn alot. I have been modifying perl/cgi scripts for about a year and
now am in the process of actually trying to write one that works. I am not
familiar with what is good and bad habits when it comes to Perl/CGI so I am
hoping that someone can critique this and let me know how I can make this
code even better!!!

Thank you,
Shawn Mulligan

#!/usr/bin/perl
use CGI::Carp qw(fatalsToBrowser);
my ($date_num, $month) = (localtime $time)[3,4];
$month ++;
my $ipfile = sprintf '%02d%02d.vipd', ++$month, $date_num;
($slash, $document) = split(//, $ENV{'DOCUMENT_URI'});
if ($slash eq "/") { $ThisPage = $ENV{'DOCUMENT_URI'}; }
else { $ThisPage = "/$ENV{'DOCUMENT_URI'}"; }
if(-e $ipfile){
open(ip, "+<$ipfile") || die ("Can't open $ipfile\n");
}
else
{
open(ip, "+>>$ipfile") || die ("Can't open $ipfile\n");
}

while (<ip>) {
chop;
($ip, $file) = split(/::/, $_);
$ip{$file} = $ip;}
$ip{$ThisPage}++;
seek(ip, 0, 0);
foreach $file (keys %ip) { print ip $ip{$file}, "::", $file, "\n";
}
close(ip);
}

 

[Eric J. Roode]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I was told that this is a
waste of time and why am I trying to re-invent the wheel, but by re
inventing the wheel you learn alot.

I completely agree. Good for you.

I have been modifying perl/cgi
scripts for about a year and now am in the process of actually trying
to write one that works. I am not familiar with what is good and bad
habits when it comes to Perl/CGI so I am hoping that someone can
critique this and let me know how I can make this code even better!!!

Glad to help. I've been writing Perl CGI scripts for a living for seven
years now. I hope my insight will be valuable to you.

#!/usr/bin/perl

For CGI scripts, in general, you want to add "-T" to the above command-
line. Not necessary, but it catches some potential security errors.

Also, before any other code, I would strongly recomment you add:

use strict;

This will catch a lot of coding errors.

use CGI::Carp qw(fatalsToBrowser);
my ($date_num, $month) = (localtime $time)[3,4];
$month ++;
my $ipfile = sprintf '%02d%02d.vipd', ++$month, $date_num;

I'm going to assume that you know that the month doesn't need to be
incremented twice, and that the fact you did so above is a simple typo or
lapse of attention.

($slash, $document) = split(//, $ENV{'DOCUMENT_URI'});

Use 'my'.

You're looking at the first two characters of DOCUMENT_URI? Is that right?
Sounds fishy.

if ($slash eq "/") { $ThisPage = $ENV{'DOCUMENT_URI'}; }
else { $ThisPage = "/$ENV{'DOCUMENT_URI'}"; }

Ah, you're really only looking at the first character. Ditch $document.
Better yet, replace the above three lines with something like

$ThisPage = $ENV{DOCUMENT_URI};
$ThisPage = "/$ThisPage" unless $ThisPage =~ /^\//;

Or, since typing $ThisPage gets tedious, a useful idiom is:

$ThisPage = $ENV{DOCUMENT_URI};
for ($ThisPage) { $_ = "/$_" unless /^\// }

if(-e $ipfile){
open(ip, "+<$ipfile") || die ("Can't open $ipfile\n");
}
else
{
open(ip, "+>>$ipfile") || die ("Can't open $ipfile\n");
}

If the file doesn't exist, you want to append to it?

ALWAYS include $! in your file open error messages -- otherwise, you'll
just be guessing as to WHY the file couldn't be opened.

Also, I would suggest varying the error messages, so you know which
statement (the "then" or the "else") generated the error.

while (<ip>) {
chop;

Use "chomp", not "chop".

($ip, $file) = split(/::/, $_);
$ip{$file} = $ip;}

Is the } a typo?

You never declared %ip in a 'my' statement.

$ip{$ThisPage}++;
seek(ip, 0, 0);
foreach $file (keys %ip) { print ip $ip{$file}, "::", $file, "\n";
}
close(ip);

Wait -- you're closing the file while looping over it?

}

It looks like if the file doesn't exist when the program is run, nothing
ever gets written to it. Is that correct?

- --
Eric
$_ = reverse sort $ /. r , qw p ekca lre uJ reh
ts p , map $ _. $ " , qw e p h tona e and print

-----BEGIN PGP SIGNATURE-----
Version: PGPfreeware 7.0.3 for non-commercial use <http://www.pgp.com>

iQA/AwUBP5XxImPeouIeTNHoEQIkNACfcO1gRRlR6nC9S8aOaQ8pgFZPulMAnjhi
1XbisoPxvf+n0O6h2qmvwUNW
=wR8e
-----END PGP SIGNATURE-----

 

[Tad McClellan]

if(-e $ipfile){
open(ip, "+<$ipfile") || die ("Can't open $ipfile\n");
}
else
{
open(ip, "+>>$ipfile") || die ("Can't open $ipfile\n");
}

[snip]

Also, I would suggest varying the error messages, so you know which
statement (the "then" or the "else") generated the error.

Or simply leave off the \n in die()'s argument and let perl tell
you which one it was.

 

[Tad McClellan]

hoping that someone can critique this and let me know how I can make this
code even better!!!

#!/usr/bin/perl

Enable taint checking:

#!/usr/bin/perl -T

Enable warnings:

use warnings;

Enable strictures:

use strict;

($slash, $document) = split(//, $ENV{'DOCUMENT_URI'});

This probably does not do what you think it does, judging by
your choice of variable name.

Did you mean to give split() a 3rd argument?

if ($slash eq "/") { $ThisPage = $ENV{'DOCUMENT_URI'}; }
else { $ThisPage = "/$ENV{'DOCUMENT_URI'}"; }

You can replace the 3 lines above with:

my $ThisPage = $ENV{'DOCUMENT_URI'};
$ThisPage =~ s#^([^/])#/$1#; # or s#^(?!/)#/#;

open(ip, "+>>$ipfile") || die ("Can't open $ipfile\n");

Your code will suddenly stop working when you upgrade to a new
Perl version that introduces a new function named ip().

Use UPPER CASE filehandles to guard against that.

seek(ip, 0, 0);

What if the seek() fails?

You don't always get what you asked for, so it is best to check:

seek(IP, 0, 0) or die "could not seek to start of file $!";