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

    #!/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);
    }
     
    kazack, Oct 22, 2003
    #1
    1. Advertising

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

    "kazack" <> wrote in
    news:eyklb.5727$:

    > 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-----
     
    Eric J. Roode, Oct 22, 2003
    #2
    1. Advertising

  3. Eric J. Roode <> wrote:
    > "kazack" <> wrote in
    > news:eyklb.5727$:



    >> 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
    Perl programming
    Fort Worth, Texas
     
    Tad McClellan, Oct 22, 2003
    #3
  4. kazack <> 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
    Perl programming
    Fort Worth, Texas
     
    Tad McClellan, Oct 22, 2003
    #4
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Sullivan WxPyQtKinter

    CGI redirection: let us discuss it further

    Sullivan WxPyQtKinter, Mar 27, 2006, in forum: Python
    Replies:
    9
    Views:
    2,175
    Sullivan WxPyQtKinter
    Mar 29, 2006
  2. Zack

    Critique of first python code

    Zack, Feb 8, 2008, in forum: Python
    Replies:
    9
    Views:
    414
    George Sakkis
    Feb 17, 2008
  3. Chad
    Replies:
    2
    Views:
    104
    Gunter Schelfhout
    Aug 14, 2003
  4. kath
    Replies:
    4
    Views:
    703
    J. Gleixner
    Apr 9, 2007
  5. Ian
    Replies:
    16
    Views:
    568
Loading...

Share This Page