Another way is to refactor them into four separate classes, based on your $dcr_type variable. A fifth class, would be used as a sort of factory and would decide which subclass to return based on the value of the $dcr_type. You would still have the if...then logic, but it is hidden by a separate layer of abstraction. It would go something like this (this is just an example, so the code below is very skimpy)

package factory;

#We don't need any logic in this constructor, so...
sub new {
   
    my $class = shift;
    bless {}, $class;
}

sub getInstance {
    my ($class, $dcr_type, @row) = ( $_[0], uc $_[1], @{$_[2]} );
    if ($dcr_type eq "CONTACT"){
        return contact->new(@row);
    }
    elsif($dcr_type eq "LITERATURE"){
        return literature->new(@row);
    }
    elsif($dcr_type eq "FAQ"){
        return faq->new(@row);
    }
    elsif($dcr_type eq "ORGANIZATION"){
        return organization->new(@row);
    }
}

package contact;

sub new {...}

sub doTheDeed{
    return ('HQ' , undef);
}

package literature;

sub new {...}

# @_row is this classes local version of the @row parameter
sub doTheDeed{
    return (undef, $_row[4])
}

package faq;
sub new {...}

# @_row is this classes local version of the @row parameter
sub doTheDeed{
    return (undef, $_row[2] eq 'Service' ? $_row[3] : $_row[4]);
}

package organization;
sub new {...}


# @_row is this classes local version of the @row parameter
sub doTheDeed{
    my @ret = ($_row[2], undef);
    $ret[0] = 'HQ' if ($_row[0] eq 'Headquarters');
    return @ret;
}
[download]

Now your main body code will look like:

my $fact = factory->new();
my $instance = $fact->getInstance($dcr_type, @row);
return $instance->doTheDeed();
[download]

All those if/elsif's make your code look somewhat like 'The Wrong Way' to do it according to the Patterns in Perl web site. I'm not sure how you would fit this problem into something resembling 'The Right Way' though I have an idea. If dcr_type is tainted, you'd have to make sure that it matches '^(CONTACT|FAQ|LITERATURE|ORGANIZATION)$', then assuming that you have those four classes defined, you can just call $dcr_type->new(@args). You could even do $dcr = s/(.*)/\u\L$1/ so you can have normal ucfirst type class names. Keep in mind I haven't really thought much about the OP's problem, so I don't know how appropriate any of this is for this particular case :-)

It would be the wrong way if it were all one class. In this case, the if is in the factory class. In the example, there is one class who's behaviors change based on the values in the class. What I have above is a factory class deciding what subclass to return based on the value passed in. Each class, then, behaves differently.

# return a list consisting of correct division and service
# as a function of $dcr_type and @row
sub division_service {
    my $dcr_type = uc shift;
    my $row = shift; # you were copying an array here. not necessary

    if    ($dcr_type eq 'CONTACT') {
        return ('HQ' , undef);
    }
    elsif ($dcr_type eq 'FAQ') {
        return (undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4])
+;
    }
    elsif ($dcr_type eq 'LITERATURE') {
        return (undef, $row->[4]);
    elsif ($dcr_type eq 'ORGANIZATION') {
        return (($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), unde
+f);
    }
    else {
        croak "Unknown \$dcr_type '$dcr_type'";
    }
}
[download]

# return a list consisting of correct division and service
# as a function of $dcr_type and @row
sub division_service {
    my $dcr_type = uc shift;
    my $row = shift;

    return
        $dcr_type eq 'CONTACT' ?
            ('HQ' , undef) :
        $dcr_type eq 'FAQ' ?
            (undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4]) :
        $dcr_type eq 'LITERATURE' ?
            (undef, $row->[4]) :
        $dcr_type eq 'ORGANIZATION' ?
            (($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), undef) 
+:
        croak "Unknown \$dcr_type '$dcr_type'";
}
[download]

# return a list consisting of correct division and service
# as a function of $dcr_type and @row
sub division_service {
    my $dcr_type = uc shift;
    my $row = shift;

    my %map = (
        CONTACT =>
            sub { 'HQ' , undef },
        LITERATURE =>
            sub { undef, $row->[4] },
        FAQ =>
            sub { undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4
+] },
        ORGANIZATION =>
            sub { ($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), un
+def },
    );

    return &{$map{$dcr_type} || croak "Unknown \$dcr_type '$dcr_type'"
+}();
}
[download]

BEGIN {
    my ($row, %map);

# return a list consisting of correct division and service
# as a function of $dcr_type and @row
    sub division_service {
        my $dcr_type = uc shift;
        $row = shift;
        return &{$map{$dcr_type} || croak "Unknown \$dcr_type '$dcr_ty
+pe'"}();
    }

    %map = (
        CONTACT =>
            sub { 'HQ' , undef },
        LITERATURE =>
            sub { undef, $row->[4] },
        FAQ =>
            sub { undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4
+] },
        ORGANIZATION =>
            sub { ($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), un
+def },
    );
}
[download]

That's what I'd do at first glance. I haven't got a few decades of experience yet, so if any of those who do can point out some maintainability flaw in this reasoning I'd be glad to listen.

Oh, and I'd name the sub something like get_division_service_for or something similar, so that it's clearer what's happening when someone reads the call.

Update: Added BEGIN to the anonmyous block in the last sample as per Anonymonk's remark.

Makeshifts last the longest.

Maintainability flaw: using an anonymous array rather than an anonymous hash or named constants. Can you tell me what 3 and 4 mean in your code?

Also a gotcha, this function will work in a module and may not when buried in the body of your program because %map isn't yet populated. Solutions: put a BEGIN in front of the start of your block, be aware of the limitation, or put a check in division_service that populates %map if it isn't populated yet.

And style points. I don't like &{...}() as much as (...)->(). Also I don't know what I think of your cute trick with putting the error check inside the dereference rather than doing a traditional exists test.

As far as the array is concerned, since I don't know the meaning of its fields I had to work with what princepawn gave us.

Thanks for pointing out the module vs main program issue which I omitted even though I was aware of it. I'd prefer the BEGIN in that case: no checks at runtime (CPU efficiency), no extra code to read and write (programmer efficiency).

You are right about &{$ref}() and I usually prefer $ref->() as well. In this case it was a conscious choice because I felt it read better this way considering I embedded the croak in the lookup.

I generally dislike exists tests because they usually force me to spell out the same hash lookup twice. At least in this case we can get away with a simple or-test, because a hash value will either not exist or be a coderef, which by definition is true. Now the choice is to either embed the error check in the call like I did (ugly), or use a temporary variable like so - also ugly:

my $lookup_div_serv = $map{$dcr_type} or croak "...";
return $lookup_div_serv->();
[download]

I'd be happy with this if I needed the $lookup_div_serv in muliple places, but I don't. This is basically a case of deciding which ugliness to consider the lesser of evils.

Makeshifts last the longest.

More discussion on if-else's at Why I Hate Nested If-Else blocks.

jeffa

L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
B--B--B--B--B--B--B--B--
H---H---H---H---H---H---
(the triplet paradiddle with high-hat)

Note how the closure for FAQ has to do argument assignment?

    CONTACT => sub {("HQ", undef)},
    FAQ     => sub {(undef, $_ [2] eq 'Service' ? $_ [3] : $_ [4])},
    ...
[download]