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 };
    }
}
[download]

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;
    }
}
[download]

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.