Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > Perl > Perl Misc > First Perl/CGI need critique on current code before further developement

Reply
Thread Tools

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

 
 
kazack
Guest
Posts: n/a
 
      10-22-2003
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);
}



 
Reply With Quote
 
 
 
 
Eric J. Roode
Guest
Posts: n/a
 
      10-22-2003
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

"kazack" <(E-Mail Removed)> wrote in
news:eyklb.5727$(E-Mail Removed):

> 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/AwUBP5XxImPeouIeTNHoEQIkNACfcO1gRRlR6nC9S8aOaQ8pgF ZPulMAnjhi
1XbisoPxvf+n0O6h2qmvwUNW
=wR8e
-----END PGP SIGNATURE-----
 
Reply With Quote
 
 
 
 
Tad McClellan
Guest
Posts: n/a
 
      10-22-2003
Eric J. Roode <(E-Mail Removed)> wrote:
> "kazack" <(E-Mail Removed)> wrote in
> news:eyklb.5727$(E-Mail Removed):



>> 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 SGML consulting
http://www.velocityreviews.com/forums/(E-Mail Removed) Perl programming
Fort Worth, Texas
 
Reply With Quote
 
Tad McClellan
Guest
Posts: n/a
 
      10-22-2003
kazack <(E-Mail Removed)> wrote:

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


--
Tad McClellan SGML consulting
(E-Mail Removed) Perl programming
Fort Worth, Texas
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Before I get too into it: site critique? Ian HTML 16 11-06-2012 09:10 AM
Critique of first python code Zack Python 9 02-17-2008 04:07 PM
.NET developement without Net Framework Pitaridis Aristotelis ASP .Net 0 01-13-2006 01:06 PM
software developement lifecycle ichor ASP .Net 1 07-14-2005 06:27 AM
Think twice before you install "Visual Web Developer 2005 Express Edition Beta1" on your developement machine. bredal Jensen ASP .Net 5 07-07-2004 10:58 AM



Advertisments