Beefy Boxes and Bandwidth Generously Provided by pair Networks
There's more than one way to do things
 
PerlMonks  

Some suggestions on coding style - a chance to critique

by emilford (Friar)
on Jun 26, 2002 at 14:32 UTC ( #177389=perlmeditation: print w/ replies, xml ) Need Help??

Monks-

After reading Mr. Muskrat's post on good programming practices, Some advice on another's scripts, my eye grew more critical of my current Perl programming style.

I've been learning to program for about 5 years, with only about a year in Perl. I just graduated college, so I haven't had any real professional experience, just what I was taught in school (primarily C/C++). Perl I learned on my own and modeled my style after my C/C++ knowledge and the books that I learned from.

I've been re-writing most of the scripts, trying to improve them as much as possible. My knowledge of Perl has greatly increased since I initially wrote these, so I realized just how terribly written they were (hopefully they still aren't that bad :-P). I've been able to critique myself to a certain extent (minize globals, use strict, -w), but now I'm looking for critique from the gurus. What better way to learn, right?

Here is an example of my code; bash/critique away. Don't worry about hurting my feelings, if something is terribly done, I want to know, so that I won't do it again and improve my programming. If you have a suggestion, post it. If I like the suggestion, I'll use it. I may, however, prefer my own method. Either way, it's good to know.

First, let me point something out. I already know one huge mistake in the code below: not using CGI.pm. While some feel that this is a matter of preference, I've been working through Ovid's tutorial on CGI to learn to use this method. This includes taint and other security issues, so I'll be improving my code there. Anything else you guys can comment on, go for it! TIA - Eric

Code follows:

#!/usr/bin/perl -w use strict; #--------------------------------------------------------------------- +----------# # Variables #--------------------------------------------------------------------- +----------# # These variables may be modified as needed my $redirect = "http://www.yoursite.com"; # where to redirect after + form submission my $sendmail = "/usr/sbin/sendmail"; # location of +sendmail program my $subject = "Form Submission Results"; # subject +line for email sent - can also be sent as CGI parameter my @recipients = qw/webmaster@yoursite.com/; # email ad +dress to send the email to my @required = (); # comma seperated li +st of all required fields - can also be sent as CGI parameter # These variables should not need to be changed my (%formdata, $current_date, $remote_host, $remote_addr, $server_name +); #--------------------------------------------------------------------- +----------# # Main #--------------------------------------------------------------------- +----------# &parse_form (\%formdata); &set_variables (\%formdata, \@required, \$redirect, \$sendmail, \$ +subject, \@recipients); &check_variables ($redirect, $sendmail, \@recipients); &check_required (\%formdata, \@required); &get_data (\$current_date, \$remote_host, \$remote_addr, \$server_ +name); &send_email (\%formdata, \@recipients, $sendmail, $subject, $curre +nt_date, $remote_host, $remote_addr, $server_name); &redirect($redirect); #--------------------------------------------------------------------- +----------# # Subroutines #--------------------------------------------------------------------- +----------# # gets the parameters sent via CGI and stores them in the %formdata ha +sh sub parse_form { my ($formdata) = @_; my (@pairs, $buffer, $value, $key, $pair); if ($ENV{'REQUEST_METHOD'} eq 'GET') { @pairs = split(/&/, $ENV{'QUERY_STRING'}); } elsif ($ENV{'REQUEST_METHOD'} eq 'POST') { read (STDIN, $buffer, $ENV{'CONTENT_LENGTH'}); @pairs = split(/&/, $buffer); } else { print "Content-type: text/html\n\n"; print "<P>Use Post or Get"; } foreach $pair (@pairs) { ($key, $value) = split (/=/, $pair); $key =~ tr/+/ /; $key =~ s/%([a-fA-F0-9][a-fA-F0-9])/pack("C", hex($1))/eg; $value =~ tr/+/ /; $value =~ s/%([a-fA-F0-9][a-fA-F0-9])/pack("C", hex($1))/eg; $value =~s/<!--(.|\n)*-->//g; if ($$formdata{$key}) { $$formdata{$key} .= ", $value"; } else { $$formdata{$key} = $value; } } } sub set_variables { my ($formdata, $required, $redirect, $sendmail, $subject, $recipients) + = @_; @$required = split /,/, $$formdata{'required'} unless ($$formdata{ +'required'} eq ""); $$redirect = $$formdata{'redirect'} unless ($$formdata{'required'} + eq ""); $$sendmail = $$formdata{'sendmail'} unless ($$formdata{'sendmail'} + eq ""); $$subject = $$formdata{'subject'} unless ($$formdata{'subject'} eq + ""); @$recipients = split /,/, $$formdata{'recipient'} unless ($$formda +ta{'recipient'} eq ""); } sub check_variables { my ($redirect, $sendmail, $recipients) = @_; my $error_message = ""; # check the redirect link $error_message .= "<li>redirect link does not appear to be valid</ +li>" unless ($redirect =~ m!^http://(www.)?\w+\.\w\w(\w)?(.*)$!); # verify sendmail will open open (MAIL, "|$sendmail -t") or die &show_errors ("Unable to open +$sendmail: $!", 0); # verfity the recipient is valid foreach (@$recipients) { + $error_message .= "<li>$_ is not a valid address </li>" unless + ($_ =~ /^[_a-z0-9.-]+\@[_a-z0-9.-]*\.\w\w(\w)?$/i); } # show the errors if there are any &show_errors ($error_message, 0) unless $error_message eq ""; # close the MAIL program - not needed currently close (MAIL); } sub check_required { my ($formdata, $required) = @_; my $error_message = ""; # check each field in the array foreach (@$required) { $error_message .= "<li>$_ is a required field</li>" if $$formdata{ +$_} eq ""; } # show any error messages &show_errors ($error_message, 1) unless $error_message eq ""; } sub get_data { my ($current_date, $remote_host, $remote_addr, $server_name) = @_; my ($year, $month, $day); my @months = qw/January February March April May June July August Sept +ember October November December/; # get the current date ($day, $month, $year) = (localtime)[3,4,5]; $year += 1900; $$current_date = "$months[$month] $day, $year"; # get the information about who submitted the form $$remote_host = $ENV{'REMOTE_HOST'}; $$remote_addr = $ENV{'REMOTE_ADDR'}; $$server_name = $ENV{'SERVER_NAME'}; } sub send_email { my ($formdata, $recipients, $sendmail, $subject, $current_date, $remot +e_host, $remote_addr, $server_name) = @_; my $message = " ------------------------------------------------------ End of Form Submission Results ------------------------------------------------------ "; # remove unwanted data from formdata delete $$formdata{'recipient'}; delete $$formdata{'subject'}; delete $$formdata{'required'}; delete $$formdata{'redirect'}; delete $$formdata{'sendmail'}; foreach my $send_to (@$recipients) { open (MAIL, "|$sendmail -t") or die &show_errors ("Unable to open +$sendmail: $!", 0); print MAIL "To: $send_to \nFrom: webmaster\@yoursite.com\n"; print MAIL "Subject: $subject\n"; print MAIL "Form Submission Results\n"; print MAIL "(c) Eric Milford - submit_form.pl\n"; print MAIL "http://www.yoursite.com\n\n"; print MAIL "------------------------------------------------------ +\n"; print MAIL "[Date Sent] - $current_date\n"; print MAIL "[Remote Host] - $remote_host\n"; print MAIL "[Remote Address] - $remote_addr\n"; print MAIL "[Server Name] - $server_name\n"; print MAIL "------------------------------------------------------ +\n\n"; foreach my $key (keys %$formdata) { print MAIL "$key: $$formdata{$key}\n\n" unless $$formdata{$key +} eq ""; } print MAIL $message; close (MAIL) or die &show_errors ("Unable to close $sendmail: $!", + 0); } } # redirects the user to the updated index page - or wherever specified sub redirect { use CGI qw(:cgi); my $url = $_[0]; my $q = CGI->new(); print $q->redirect( -url => $url ); } sub show_errors { my ($error_message, $method) = @_; $method = "<font face=verdana size=1>An error with the script's in +stallation has occured. Please contact the webmaster with the error messages listed be +low!" if $method == 0; $method = "<font face=verdana size=1>An error occured with your su +bmission. Please check the errors and make any necessary modficiations!</font>" + if $method == 1; print "Content-type: text/html\n\n"; print "<table width=400 border=0 cellpadding=5 cellspacing=5 align +=center bgcolor=EEEEEE>\n"; print "<tr><td align=center width=100%><font face=verdana color=#5 +55555 size=1><b>There was unfortunately a problem</b></font></td></tr +>\n"; print "<tr><td>$method<br><br>"; print "<ul><font face=verdana size=1>$error_message</font></ul>"; print "</td></tr></table>\n"; exit(0); }

Edited: ~Wed Jun 26 21:02:39 2002 (GMT), by Footpad:
Actions: Added <READMORE> tag

Comment on Some suggestions on coding style - a chance to critique
Download Code
Re: Some suggestions on coding style - a chance to critique
by moxliukas (Curate) on Jun 26, 2002 at 14:49 UTC

    I'm a beginner at Perl too, so I'm not sure if my suggestions are quite relevant. However, I was surprised to see this sort of code:

    my $redirect = "http://www.yoursite.com"; my $sendmail = "/usr/sbin/sendmail"; my $subject = "Form Submission Results";

    I have always thought that I should avoid double quoted strings in expressions like these, because they take longer to parse (Perl has to do various substitutions, right?). So I would use single quoted strings in these examples. Please correct me if I am wrong, as I would be interested to read some discussion about this.

      You're right! I looked at this post and they mentioned using single quotes when no interpolation of variables is needed. They also mentioned that this is a matter of personal preference. This probably would be good practice, but in my use, I'm not sure if it makes that big a difference. I wonder how much of a time difference single quotes vs. double quotes makes? Either way, thanks for the suggestion.
        Running this code:
        use Benchmark qw(cmpthese); cmpthese(10000000, { single => sub {$foo = 'http://www.yoursite.com'}, double => sub {$foo = "http://www.yoursite.com"}, });
        produces these results on my machine:
        Rate double single double 2865330/s -- -2% single 2915452/s 2% --
        So it looks like single quotes may be slightly faster, but that may be within the margin of error. I generally use single quotes unless I need interpolation of variables, but it's probably just a matter of preference.

        -- Mike

        --
        just,my${.02}

      $ perl -MO=Deparse my $redirect = "http://www.yoursite.com"; my $sendmail = "/usr/sbin/sendmail"; my $subject = "Form Submission Results"; my $redirect = 'http://www.yoursite.com'; my $sendmail = '/usr/sbin/sendmail'; my $subject = 'Form Submission Results'; - syntax OK
      I believe this is optimized away, so I think it really is only a matter of preference. I personally prefer to use single quotes when possible though.


      Staunch

Re: Some suggestions on coding style - a chance to critique
by broquaint (Abbot) on Jun 26, 2002 at 15:05 UTC
    Just a couple of comments on coding style
    • You don't really *need* to comment what the purpose of your variables are if they're clearly labeled (they were pretty clear to me at least), although this is more of a personal preference than anything.
    • It's good practice to keep your line-length under 80 columns for several reasons
      • it shows that your expression is too long and probably needs to be simplified (as always there are exceptions)
      • 80 cols is the standard width for terms which leads on to ...
      • code is a lot easier to read when the lines are nice'n'short (again this is personal opinion)
    • Here's a good rule of thumb for the amount arguments to pass to functions - if you've got 7+ arguments you probably don't have enough[1]. I think you could reduce the amount arguments you pass about by taking advantage of perl's aggregate data types.
    • Ack - no use strict in sight! This is pretty much a must for anything but the smallest of perl programs, and the warnings pragma is also muchly helpful.
    • Is there any reason why you're passing some of your scalar variables as references? While this is helpful and sometimes necessary when dealing with hashes and arrays, scalars are passed by value so don't needed to be passed by reference (unless you've got a good reason to of course).
    And do make sure you get onto using CGI and watching out for potential security holes (see. opening a pipe to sendmail).
    HTH

    _________
    broquaint

    [1] this is indeed sarcasm but unfortunately I cannot take credit for this masterful piece of wit[2] which was introduced to me by a co-worker
    [2] that, however, is mine ;-)

      Thanks for the suggestions. First, I do use strict and -w, it just happened to get cut off when I was cutting and pasting my code.

      Also, if scalar variables are passed by reference, does this mean that they can be modified from within a subroutine? I thought that I would have to pass a reference to the scalar in order to modify them (i.e. - \$variable). Is this not true?

      About passing variables, I could push everything into a seperate data type, but wouldn't that make things a little less clear and require "not necessary" lines of code?

      Thanks for the suggestions.
        I thought that I would have to pass a reference to the scalar in order to modify them (i.e. - \$variable). Is this not true?
        You don't *have* to pass by reference to modify variables as @_ is just a list of aliases (it's magic you see) e.g
        sub foo { $_[0] = "modified in foo()"; print "in foo()\n"; } my $var = "a string"; print "\$var is $var\n"; foo($var); print "\$var is $var\n"; __output__ $var is a string in foo() $var is modified in foo()
        However passing by reference is more explicit, but that then introduces in the issue of 'dirtying' your arguments which is generally agreed to be avoided if possible. I believe the reasoning is that entering a function shouldn't modify the state of your program, although this can usually be ignored with more complex data e.g throwing around hashes is potentially pricey.
        I could push everything into a seperate data type, but wouldn't that make things a little less clear and require "not necessary" lines of code?
        At the worst it will require a line or two of code, but organising your data is far more likely to *reduce* your code. Let's take send_mail() for example. Currently you have 8 arguments, whereas I'd probably reduce that to 2 arguments with all the sendmail-related information in a hash or even a hash of hashes. But then again I think that all this throwing about of data is a symptom of a mis-organised program, to which I think part of the solution would be re-structuring the data as once you have well-defined data structures the program tends to follow.
        HTH

        _________
        broquaint

Re: Some suggestions on coding style - a chance to critique
by aersoy (Scribe) on Jun 26, 2002 at 15:11 UTC

    Hello,

    You may take a look at this node by demerphq to read about a better way of sub placement and the possible effects placing them on bottom can cause.

    --
    Alper Ersoy

Re: Some suggestions on coding style - a chance to critique
by abaxaba (Hermit) on Jun 26, 2002 at 15:21 UTC
    First off, this looks a lot like something from Mr. Wright. Bag it. Use Mime::Lite or something like that for the mailing. But kudos for using pass-by-ref. I see lots of pass-by-val, and that drives me nuts. A couple of other things, but maybe just preference:

  • If you're printing to STDOUT in various places, set $|=1; Otherwise, push everything onto an array, and just dump the whole array when you're done.
  • I don't use the C-style of bracketing. Too hard for me to read. Rather:
    sub function { # code in here }

    This keeps the brackets lined up vertically, and helps me keep track of deeply nested loops. Of course, this is certainly preference, and doesn't really have much to do wrt the code.

  • Dump the "&" in front of subroutine calls.
  • Likewise, dump the quotes in hashkeys, unless you got whitespace in there. Of course, if you have whitespace in your hashkeys, camelBack them or use _ or something. I'm all for saving keystrokes!
  • If you're using a hash, then use the hash:
    @$required = split /,/, $$formdata{'required'} unless ($$formdata{ +'required'} eq ""); $$redirect = $$formdata{'redirect'} unless ($$formdata{'required'} + eq ""); . . .

    I hate stuff like this. Assigning variables to hash values, IMHO, should only be done if you have some really complex data structures, and even then, I believe you should use some sort of interpolation, lest you continue to de-ref every time you access something.

  • Again, maybe just style: Unless it's a small hack, a never define vars outside of subroutines. You forget a my, and all hell could break loose. For more on what I'm getting at, look here
  • If/then is your friend:
    $method = "<font face=verdana size=1>An error with the script's in +stallation has occured. Please contact the webmaster with the error messages listed be +low!" if $method == 0; $method = "<font face=verdana size=1>An error occured with your su +bmission. Please check the errors and make any necessary modficiations!</font>" + if $method == 1;

    Use standard if-then/ternary syntax here. Make perl figure out which case you're on before assigning the $method values. AFAIK, in your code, it assigns the value, then will pitch it if the conditional isn't met. Don't waste the cycles.

  • HTH, and again, much of this may be just preference.
    ÅßÅ×ÅßÅ
    "It is a very mixed blessing to be brought back from the dead." -- Kurt Vonnegut

      Dump the "&" in front of subroutine calls.

      What is the reason for this? Does this make some sort of difference?

      If you're using a hash, then use the hash:
      @$required = split /,/, $$formdata{'required'} unless ($$formdata{ +'required'} eq ""); $$redirect = $$formdata{'redirect'} unless ($$formdata{'required'} + eq ""); . . .
      I hate stuff like this. Assigning variables to hash values, IMHO, should only be done if you have some really complex data structures, and even then, I believe you should use some sort of interpolation, lest you continue to de-ref every time you access something


      I'm not positive what you're refering to here. Is it just that I should use the hash in my code as opposed to assigning the hash values to a variable and then using the variable instead?

      If you're printing to STDOUT in various places, set $|=1; Otherwise, push everything onto an array, and just dump the whole array when you're done

      What exactly does the $|=1 do?

      Thanks for the suggestions.
        Dump the "&" in front of subroutine calls.
        1. Using & disables a function's prototype.
        2. If you don't pass any parameters to a function, but call it using &, it will receive your current @_ as its parameters. This can lead to very hard to spot errors.
        Is it just that I should use the hash in my code as opposed to assigning the hash values to a variable and then using the variable instead?
        Indeed. Why assign to a hash first when you're going to put it into a variable afterwards? Using a hash also is usually cleaner, since it provides an own little name space for the variables in question; it keeps together what belongs together, and keeps them from littering the global namespace.
        What exactly does the $|=1 do?
        It disables buffering for the currently selected filehandle (which would usually be STDOUT). See perlvar.

        Makeshifts last the longest.

      # If you're printing to STDOUT in various places, set $|=1;

      Why would you want to slow things down to get output in smaller chunks more often? Buffering is enabled for a reason, and I'm curious as to why you recommend disabling it.

Re: Some suggestions on coding style - a chance to critique
by lestrrat (Deacon) on Jun 26, 2002 at 15:34 UTC

    I personally hate this style script:

    #!perl my $var = ...; my @var = ...; sub1(); sub2(); exit 0;

    I think this leads to a lot of global variable usage and sloppy code in general. I personally prefer using a single entry point, and keeping everything in there

    #!perl main(); sub main { my $var = ...; my @var = ...; sub1($var); sub2(@var); exit 0; }

    Don't know about you, but I find this less error prone.

    I also think that the $$hash{ element } notation is confusing.... It's not that much different, but I think @{ $hash->{ element } } would be easier to read than  @{ $$hash{ element } } or some such thing.

      I agree with your style of coding, as it does look a little cleaner and seems that it would prevent the accidental use of global variables. The only concern I have with that is if I were to give the script to someone else with no programming knowledge, they'd have to sort through lines of code to make any necessary changes to variables. When the variables are up top, it's the first thing that they see, making it easier for them to change the variables and less likely for them to screw something up.

        True. For a standalone script, it's always nice to see some globally used parameters at the top for easy customization/configuration.

        I do the follwing in such cases...

        If the variables are "configuration" type of variables, I would use constants, not plain scalars. I feel much better in knowing that most ( if not all ) of the globally accessible variables would be visually different from other local variables:

        #!perl use constant LOG_DIRECTORY => '/foo/bar/baz'; use constant SOME_URL => 'http://baboon.com'; main(); sub main { .... some_random_function( LOG_DIRECTORY, $local_variable ); }
Re: Some suggestions on coding style - a chance to critique
by Sifmole (Chaplain) on Jun 26, 2002 at 15:39 UTC
    One note about style:
    You might find it cleaner, and I believe it is quicker, if you group your print statements or use a "heredoc".
    print "abc\n"; print "def\n"; print "ghi\n"; becomes print "abc\n", "def\n", "ghi\n"; or print << EOD; abc def ghi EOD
    You can do this as well with your  print MAIL section.

    You have some pretty long parameter lists, you might want to consider named parameters.

    Get rid of temporary variables:

    use CGI qw(:cgi); my $url = $_[0]; my $q = CGI->new(); print $q->redirect( -url => $url ); # becomes my $q = CGI->new(); print $q->redirect( -url => $_[0] );

    You use a lot of post-conditionals, and I tend to as well, but you may want to re-evaluate whether some of them make the code more difficult to read than others. A test is whether the condition or the result is "more important" and use that to determine which goes first.

    &get_data (\$current_date, \$remote_host, \$remote_addr, \$server_name); #... sub get_data { my ($current_date, $remote_host, $remote_addr, $server_name) = @_; #.... # get the information about who submitted the form $$remote_host = $ENV{'REMOTE_HOST'}; $$remote_addr = $ENV{'REMOTE_ADDR'}; $$server_name = $ENV{'SERVER_NAME'}; }
    Why bother passing in the references? Just return the scalar values:
    ($current_date, $remote_host, $remote_addr, $server_name) = get_data(); sub get_data { #... # get the current date ($day, $month, $year) = (localtime)[3,4,5]; $year += 1900; return ( "$months[$month] $day, $year", $ENV{'REMOTE_HOST'}, $ENV{'REMOTE_ADDR'}, $ENV{'SERVER_NAME'}; }

    And just to restate, use CGI;

    Hope this is useful.

Re: Some suggestions on coding style - a chance to critique
by FoxtrotUniform (Prior) on Jun 26, 2002 at 15:39 UTC

    One minor point: /^[_a-z0-9.-]+\@[_a-z0-9.-]*\.\w\w(\w)?$/i is not an especially robust way of checking email addresses for validity. Use Email::Valid or Mail::CheckUser instead.

    --
    The hell with paco, vote for Erudil!
    :wq

Re: Some suggestions on coding style - a chance to critique
by Aristotle (Chancellor) on Jun 26, 2002 at 15:59 UTC
Re: Some suggestions on coding style - a chance to critique
by Juerd (Abbot) on Jun 26, 2002 at 22:18 UTC

    For something this simple, I would not use subroutines. It only makes things harder to read and harder to maintain if you use subs for code that gets executed only once. A rule of thumb: subroutines should return something and the return value must be useful.

    You let the web user define the path to the sendmail program. That is EXTREMELY INSECURE. Imagine what would happen if someone has rm -rf /; as $formdata{sendmail}.

    Here's a complete (UNTESTED) rewrite of your script, with some suggestions and remarks:

    
    #!/usr/bin/perl -w
    
    # This script has never been tested
    
    use CGI;
    use strict;
    
    # Suggested improvement: use a hash for these
    
    # URL to redirect to after form submission
    my $redirect = 'http://www.yoursite.com/';
    
    # Path to the sendmail executable
    my $sendmail = '/usr/sbin/sendmail';
    
    # Default subject
    my $subject = 'Form Submission Results';
    
    # List of recipients
    my @recipients = ('webmaster@yoursite.com');
    
    # Required fields
    my @required = ();
    
    
    my $cgi = CGI->new();
    
    my %formdata = $cgi->Vars;
    
    
    sub set {
        my ($thing) = @_;
        return defined $thing and length $thing ? 1 : 0;
    }
    
    @required   = split /,/, $formdata{required}  if set $formdata{required};
    $redirect   =            $formdata{redirect}  if set $formdata{redirect};
    #  !!! NEVER EVER LET THE USER CHOOSE THE PROGRAM THAT GETS EXECUTED !!!
    $subject    =            $formdata{subject}   if set $formdata{subject};
    @recipients = split /,/, $formdata{recipient} if set $formdata{recipient};
    
    CHECK: {
        my @errors;
        push @errors, 'Redirection URL seems to be invalid'
            unless $redirect =~ m!^http://!;
        for (@recipients) {
            push @errors, "E-Mailaddress $_ seems to be invalid"
                unless /^[\w.-]+\@([\w.-]+\z/;
        }
        for (@required) {
            push @errors, "Field '$_' is a required field" unless set $formdata{$_};
        }
        last CHECK unless @errors;
    
        # Suggested improvement: a templating module
        print "Content-Type: text/html\n\n";
        print '<html><body><h1>Errors:</h1><ul>', map "<li>$_</li>", @errors;
        print '</ul></body></html>';
    
        exit;
    }
    
    
    # E-Mail already has a Date: header. Why bother having your own?
    
    delete @formdata{ qw/recipient subject required redirect sendmail/ };
    
    # Suggested improvement: an E-mail module
    for my $recipient (@recipients) {
        open my $mail, '|-', $sendmail, '-t', '-oi' or die "Couldn't start sendmail: $!";
        print $mail join("\n",
            "To: $recipient",
            "From: webmaster\@yoursite.com",
            "Subject: $subject",
            '',
            "Remote host: $ENV{REMOTE_HOST} ($ENV{REMOTE_ADDR})",
            "Server: $ENV{SERVER_NAME}",
            '',
            map "$_: $formdata{$_}\n", keys %formdata
        );
    }
    
    $cgi->redirect( -url => $redirect );
    

    As you can see, there's still a lot of room for improvement.

    Note for PM regulars: no, I still don't like CGI.pm, but I thought it'd be a lot better than the current solution.

    - Yes, I reinvent wheels.
    - Spam: Visit eurotraQ.
    

      Thanks for the tip on allowing someone to specify their own sendmail. I didn't realize the huge security issue involved with that. I'll be sure to make changes on that asap.

      return defined $thing and length $thing ? 1 : 0;

      What exactly does this do? Is this one of those awkward ways to write an if statement as in old-style C? What is the purpose of the ? 1 : 0?

      Finally, what do you have against CGI.pm? I'm looking for both reasons to use it and reasons not to use it. You obvioulsy have a strong opinion in one direction.

      Note for PM regulars: no, I still don't like CGI.pm, but I thought it'd be a lot better than the current solution.

        Is this one of those awkward ways to write an if statement as in old-style C? What is the purpose of the ? 1 : 0?

        It's the tenary operator. Very C-ish, yes.
        The ? 1 : 0 isn't really needed here, as set() is only used in boolean context, but this clarifies the return value: true or false. Sometimes I write  !! $foo (double boolean negation), sometimes I write  $foo ? 1 : 0 , and sometimes just  $foo .

        Finally, what do you have against CGI.pm? I'm looking for both reasons to use it and reasons not to use it. You obvioulsy have a strong opinion in one direction.

        The short version:
        It is slow (huge), has awful parsing (I just don't like it), a very annoying hybrid interface and too much junk code that is there for (backwards) compatibility. And it doesn't use strict, and has many globals.

        - Yes, I reinvent wheels.
        - Spam: Visit eurotraQ.
        

Re: Some suggestions on coding style - a chance to critique
by George_Sherston (Vicar) on Jun 27, 2002 at 01:40 UTC
    Sibling monks have alluded to this but not quite spelt it out: I'd strongly encourage you to Use More Modules.

    Even in this small script you could save yourself some lines of code, improve readability, increase flexibility and guard against snafu, by using Mail::Sendmail.

    And of course (I mean *of course*) you could be using CGI.pm *much* more. The whole of parse_form() would become redundant, just to start with.

    There's such a wealth of good work in CPAN; and of course there is some time to be spent learning how to use each new module; but you get that time back almost at once as the module speeds up your work.

    And learning to use a module puts you in the way of looking over some of the best code around, which does no harm to one's programming style.

    George Sherston
Re: Some suggestions on coding style - a chance to critique
by jarich (Curate) on Jun 28, 2002 at 01:15 UTC
    Juerd had some reasonable points against the use of CGI. He said:
    The short version:
    It is slow (huge), has awful parsing (I just don't like it), a very annoying hybrid interface and too much junk code that is there for (backwards) compatibility. And it doesn't use strict, and has many globals.

    Well I personally strongly support CGI, avoiding parameter cleansing and all the rest sounds great to me. However Juerd mentions some very good reasons against, which I can't refute. So, since you don't need all the things that CGI provides perhaps you should consider CGI::Simple which is faster, uses strict and is a very clean and nice reimplementation of CGI, by our very own tachyon :).

    As everyone else says, I also recommend using more modules. Particularly something like Mail::Sendmail.

    Things like this:

    $$redirect = $$formdata{'redirect'} unless ($$formdata{'required'} eq +"");
    can be better (more clearly) written as:
    $$redirect = $formdata->{redirect} if ($formdata->{required} ne "");
    And if it's appropriate, you might write it like this instead:
    # sets unless $formdata->{redirect} is # a) undefined b) "" or c) 0 $$redirect = $formdata->{redirect} if $formdata->{required}; # this sets it only if $formdata->{redirect} is defined # ($formdata->{redirect} may be "" or 0 if already # initialized to that value) $$redirect = $formdata->{redirect} if defined($formdata->{required};
    Either way, notice that the -> notation makes it much easier to see that $formdata is a reference to a hash of scalars. I've changed your unless eq to an if ne because in my experiences it's easier for someone to read. Trailing unlesses are great if the condition is simple such as next unless $name; but they start cluttering things if the reader has to evaluate the condition in their head and then take the complement of that.

    Another style note, is that your indentation really ought to be consistant, although I hope that the inconsistancies I see here are due to cut-n-paste effects, for example "sent_email" is very difficult to read.

    Hope it helps.

    jarich

    Update: Juerd is of course correct, the two lines are technically equivalent. I'd choose the version I've suggested because I expect that none of my students would understand the first straight off. Of course few of the students would understand the second straight off either... but it'd be easier for me to explain. ;)

      $$formdata{redirect} can be better written as: $formdata->{redirect}

      Technically, they're equivalent, so the second cannot be better. In my opinion, the arrow-notation is clearer, but it's still just a matter of taste.

      - Yes, I reinvent wheels.
      - Spam: Visit eurotraQ.
      

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://177389]
Approved by broquaint
Front-paged by rob_au
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others imbibing at the Monastery: (6)
As of 2014-12-26 00:34 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    Is guessing a good strategy for surviving in the IT business?





    Results (163 votes), past polls