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

Discussion in 'Perl Misc' started by kazack, Oct 22, 2003.

  1. kazack

    kazack Guest

    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

    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");
    open(ip, "+>>$ipfile") || die ("Can't open $ipfile\n");

    while (<ip>) {
    ($ip, $file) = split(/::/, $_);
    $ip{$file} = $ip;}
    seek(ip, 0, 0);
    foreach $file (keys %ip) { print ip $ip{$file}, "::", $file, "\n";
    kazack, Oct 22, 2003
    1. Advertisements

    Hash: SHA1

    I completely agree. Good for you.
    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.

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

    Use 'my'.

    You're looking at the first two characters of DOCUMENT_URI? Is that right?
    Sounds fishy.
    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 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.
    Use "chomp", not "chop".
    Is the } a typo?

    You never declared %ip in a 'my' statement.
    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?

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

    Version: PGPfreeware 7.0.3 for non-commercial use <>

    -----END PGP SIGNATURE-----
    Eric J. Roode, Oct 22, 2003
    1. Advertisements

  3. Or simply leave off the \n in die()'s argument and let perl tell
    you which one it was.
    Tad McClellan, Oct 22, 2003

  4. Enable taint checking:

    #!/usr/bin/perl -T

    Enable warnings:

    use warnings;

    Enable strictures:

    use strict;

    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?

    You can replace the 3 lines above with:

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

    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.

    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, Oct 22, 2003
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.