Beefy Boxes and Bandwidth Generously Provided by pair Networks
Keep It Simple, Stupid
 
PerlMonks  

CGI Design

by Dogma (Pilgrim)
on Apr 06, 2002 at 22:15 UTC ( #157187=perlquestion: print w/ replies, xml ) Need Help??
Dogma has asked for the wisdom of the Perl Monks concerning the following question:

I'm writting a cgi that returns multiple pages through the process of modifing a database. This code snippet controls which page body is returned. My question to fellow monks is this considered good programming practice? Is this is terrible design? Is there a more "modern" practice I should be using?
if (defined($q->param())) { if ($q->param('action') eq 'add') { if ($q->param('confirm')) { if ($q->param('commit')) { &commit_dialog(); } else { &confirm_dialog(); } } else { &add_dialog(); } } elsif ($q->param('action') eq 'remove') { if ($q->param('confirm')){ if ($q->param('commit')) { &commit_dialog(); } else { &confirm_dialog(); } } else { &choose_dialog(); } } elsif ($q->param('action') eq 'modify') { if ($q->param('confirm')) { if ($q->param('commit')) { &commit_dialog(); } else { &confirm_dialog(); } } else { &choose_dialog(); } } else { &action_dialog(); } } else { &action_dialog(); }
<code>

Comment on CGI Design
Download Code
Re: CGI Design
by cjf (Parson) on Apr 06, 2002 at 22:21 UTC
Re: CGI Design
by Ovid (Cardinal) on Apr 06, 2002 at 23:15 UTC

    I have to agree with cjf. When you have a complicated series of if/elsif/else statments, it can be tricky to track everything down and a hash is often a good choice. My personal thought would be to use a dispatch table. Encapsulate it into a subroutine that takes the query object and returns a reference to the correct function based upon the user's selection. Here's a rough example.

    #!/usr/bin/perl -wT use strict; use CGI; my $query = CGI->new; # get appropriate function my $function = get_branch_function( $query ); # get results and process my $results = $function->( $query ); sub get_branch_function { my $query = shift; my $action = $query->param('action') || 0; my $confirm = $query->param('confirm') ? 1 : 0; my $commit = $query->param('commit') ? 1 : 0; my %dispatch = ( add => { confirm => { commit => \&commit_dialog, default => \&add_dialog, }, default => \&add_dialog, }, remove => { confirm => { commit => \&commit_dialog, default => \&confirm_dialog, }, default => \&choose_dialog, }, modify => { confirm => { commit => \&commit_dialog, default => \&confirm_dialog, }, default => \&choose_dialog, }, default => \&action_dialog ); if ( ! exists $dispatch{ $action } ) { return $dispatch{ default } } elsif ( ! $confirm ) { return $dispatch{ $action }{ default }; } elsif ( ! $commit ) { return $dispatch{ $action }{ confirm }{ default }; } else { return $dispatch{ $action }{ confirm }{ commit }; } }

    Now, everything is nicely tucked away. If you find you need to change the defaults, it's easy to see where they are. If you find you need to completely rework how you determine the correct function, it's all nicely stuffed into a subroutine that takes one parameter that is unlikely to change, and one return type (subref) that is unlikely to change (the reference type is unlikely to change, that is, not the referent).

    Note that the subroutine does not depend on anything declared outside of itself. This is useful because then it's easy to muck around with the code and minimize breakage. However, in your example, you are calling subroutines with no arguments and no return types. This can cause problems because clearly you are using variables defined outside of the lexical scope of the subs. If you wind up calling several subs, you'll find your self wondering exactly what changed the value of $foo, since $foo can be changed anywhere.

    The nice thing about encapsulating everything is that each subroutine can then do anything it wants to with its variables without worrying about the effect on the rest of the program. That's a huge win :)

    Further, by having every sub explicitly return everything, it's very clear what's going on. You just have a bunch of mysterious sub calls with no idea what data they need or generate. Here's a contrived example of this.

    sub foo { if ( $temp > 7 ) { $temp--; $level = $temp / $difference1; } else { $level = $temp / $difference2; } }

    I hate having to maintain code like that (I should know, I've written some code like that). Not only is it not clear what's going on, but the first time I get a divide by zero error, I'm going to scream when I try to figure out which of myriad subs set $temp to zero. What happens if I realize that $temp should be named $temperature because my coworker keeps thinking it's a temp variable? Well, now I have to change it everywhere because I didn't pass the variable in. Further, I can't reuse this function anywhere else without synchronizing variable names.

    I should add that the only reason I went into this level of detail because you asked :) I hope it helps.

    Cheers,
    Ovid

    Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

      However, in your example, you are calling subroutines with no arguments and no return types.

      You caught me writting coupled code. The reason being that I'm still using a CGI object and that I'd have to pass a reference into every single function to make it truely decoupled. I was going to be lazy and move the functions into a module and then pass the CGI object into that namespace through a constructor. However I think I'll look into a dispatch model along the lines you suggest.

      Thanks,

      -Dogma

      Or going in an OOPy direction from the multiple sub sugguestion, you could implement the different behaviors in different classes. You use a factory method to create an instance of the apropriate class based on what action is to be taken (ConfirmClass, CommitClass) and then call the handleRequest method (probably passing along the contents of $cgi->params or what not).

      Something to be mindful of is the possibility of no corresponding entry in the dispatch table, either by accident (ie, a typo), negligence, or something more sinister.

      A quick fix could be as simple as checking that get_branch_function's returned something true...

      # get appropriate function my $function = get_branch_function( $query ) or die "Holy Missing Function, Batman!";

      ... while the more paranoid among us could then use ref or something to check it was something that could actually be run with $function->().

          --k.


Re: CGI Design
by rob_au (Abbot) on Apr 07, 2002 at 00:35 UTC
    I think we've all been down this path before and found our code wanting for a cleaner framework than nested if-elseif blocks. For this type of scenario, I would strongly recommend looking at CGI::Application (which I have reviewed previously here) - This module offers a very simple, but expansive object-orientated framework that provides some structured order to CGI scripts written in this manner.

    The advantage which the CGI::Application interface offers is that individual "run-modes" and execution blocks can be defined, each correlating to a particular stage of your CGI interface. Using the CGI::Application "run-mode" interface, the code above could be re-written as follows - Note that some liberty has been taken with the subroutine names and the example code shortened to reflect only that snippet of code shown above:

    sub setup { my $self = shift; $self->mode_param('action'); $self->run_modes({ 'AUTOLOAD' => 'action_dialog', 'add' => 'add_dialog', 'add_commit' => 'add_commit_dialog', 'modify' => 'modify_dialog', 'modify_commit' => 'modify_commit_dialog', 'remove' => 'remove_dialog', 'remove_commit' => 'remove_commit_dialog' }); }

    Furthermore, the CGI::Application module implements methods to address the CGI and HTML::Template modules, facilitating powerfule methods for the construction of extensible web applications.

    In short, I would strongly look into CGI::Application for this type of application or alternatively, some of the different web application frameworks reviewed by princepawn at Web Application Frameworks and their Templating Engines with a Comparative Study of Template and HTML::Template.

    Good luck.

     

      Furthermore, the CGI::Application module implements methods to address the CGI and HTML::Template modules, facilitating powerfule methods for the construction of extensible web applications.

      Well tell me how you really feel about CGI::Application. :) I'll take a look at it, do you know if there are any traps using it under mod_perl?

      Thanks,

      -Dogma

        Nope. I use it extensively with mod_perl and it works beautifully. In fact, if you use this module as it's intended, I think it makes mod_perl programming much easier, as it makes it easy to avoid globals.

        I love the dynamic duo of CGI::Application and HTML::Template. They make it very easy to create flexible and maintainable apps that separate code from content.

        -Any sufficiently advanced technology is
        indistinguishable from doubletalk.

Re: CGI Design
by Anonymous Monk on Apr 07, 2002 at 09:51 UTC
    Check out CGI::Application. You may find some great ways to clean up your code and use better dispatch methods.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://157187]
Approved by cacharbe
Front-paged by Dog and Pony
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others studying the Monastery: (11)
As of 2014-09-23 09:11 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    How do you remember the number of days in each month?











    Results (216 votes), past polls