http://www.perlmonks.org?node_id=89359

deriwana has asked for the wisdom of the Perl Monks concerning the following question:

Could you guys take a peek at my code?
I would appreciate any suggestions on bugs, bad syntax,etc.
As you can tell, it's just a script that retrieves records from a flat-file database.
THANKS
#!usr/local/bin/perl -w use CGI; $query =new CGI; $guest_file = "guests.txt"; &print_page_start; if ($query->param()) { $search_name = $query->param('search_name'); eval { open (GUESTS, "< $guest_file") or die "Can't open $guest_file: $!"; while (<GUESTS>) { chomp; ($name, $email, $comments) = split /::/; if ($name =~ /$search_name/i { print "$name<BR>\n$email<BR>\n$comments<BR>\n<HR>\n"; } } } } else { print "<FORM>\n"; print "<INPUT TYPE=\"text\" NAME=\"search_name\"><BR>\n"; print "<INPUT TYPE=\"submit\">\n"; print "</FORM>\n"; } chomp $@; if ($@) { print "ERROR: $@<BR>\n"; } &print_page_end; sub print_page_start { print $query->header; print "<HTML>\n<HEAD>\n<TITLE>Search for Records</TITLE>\n"; print "</HEAD>\n<BODY>\n"; print "<H1>Search for Records</H1>\n; } sub print_page_end { print "</BODY>\n</HTML>\n"; }

Replies are listed 'Best First'.
Re: C any bugs?
by jeroenes (Priest) on Jun 18, 2001 at 21:19 UTC
    1. use strict or die (updated)
    2. declare your variables with my
    3. Try CGI's builtin functions for generating the header, form, tags and actually all html.  use CGI qw/:html3/
    4. I wouldn't use eval to trap errors. There is an option for CGI like use CGI::Carp 'fatalsToBrowser'; (updated).
    5. The while loop could be done more concise:
      while( <GUESTS> ){ #no chomp, we need the \n later on... split /::/; if( $_[0] =~ /$search_name) { print join "<BR>\n", @_; print "<HR>\n"; } }
    6. Try use diagnostics;, maybe you like it. I do.
    Jeroen
    </code> "We are not alone"(FZ)
Re: C any bugs?
by buckaduck (Chaplain) on Jun 19, 2001 at 00:29 UTC
    On general principle, I believe all CGI programs should run with the -T flag to enable taint mode (see perlsec). That's not a serious issue here, because your code doesn't use tainted data in an unsafe manner -- at least, not yet. But it could grow, so it doesn't hurt to be careful early on.

    I would also be more comfortable if your regex did something like this:

    $name =~ /\Q$search_name\E/i
    To make sure that special characters in your CGI parameter don't accidentally do something you didn't intend. See quotemeta and perlre.

    buckaduck

Re: C any bugs?
by murphya (Sexton) on Jun 18, 2001 at 22:04 UTC
    Quickly, why mix up subroutines and large chunks of code? May I suggest something like:

    &sub_one; &sub_two; &sub_three; sub sub_one{ } sub sub_two{ } sub sub_three{ }

    I think that this is clearer and easier to understand. It maximises the benefit of using subroutines.

    Other than that I agree with the above comments on CGI and strict. CGI is very important from an upkeep point of view - so long as you keep your CGI module up to date you don't have to worry about rewriting your code to match the latest standards. strict is a very useful learning tool.

    "The significant problems we face in life can not be solved at the same level of thinking we were at when we created them." -Albert Einstein

      Unless you have a specific reason to use the implicit argument list, it is best to use parens in calling functions.

      If this comment made no sense to you, the following summary may help.

      Also functions should be given names that clearly describe what they do. Inability to find a meaningful name for a function is usually an excellent indicator that the code is not well factored. Certainly having a function whose name does not properly describe what it does is almost always a maintainance problem...

Re: C any bugs?
by Anonymous Monk on Jun 18, 2001 at 21:48 UTC
    Above comments withstanding, here it goes:

    Change this:

    if ($query->param()) { $search_name = $query->param('search_name'); ...
    into:
    if( $search_name = $query->param('search_name') ) { ...
    If the param('search_name') is empty/fails, it will return a blank string, '', which perl treats as a false value, i.e., if condition fails. Same effect as before, a little clearer.

    Why are you doing <BR> and \n here? \n won't have any real effects on the html(usually), so it's not needed. Unless, I presume, you need to edit it, and this is for 'beautification purposes'. If you really want/need to line breaks, try <p>. This is where I'm talking about:

    print "$name<BR>\n$email<BR>\n$comments<BR>\n<HR>\n";

    Major Recommendation: Look up CGI.pm's HTML handling abilities! Especially its abilities with forms. It'll make your pages a lot less error prone, and the CGI a lot more bearable/easier to read for larger examples. For example, <title>this is the title</title> can be re-written, assuming $q is a cgi object, as $q->title("This is the title"). CGI will automatically add an /title, or end whatever tag, as needed. Look into it.

      This was my post(I guess I got logged out somehow). Just thought I'd clear up any confusion.