if ($mode = 'alfa') {
    push @foo, [ @bar ];
} elsif ($mode = 'beta') {
     push @foo, [ @bar ] if $self->{baz} eq $gamma;
} else {
    die "Unknown mode: $mode!";
}
[download]

Thanks, CountZero! While there was something of interest in all the comments, yours was the most immediately applicable because it clued me into what to do right at the three points the push call was made and thereby enabled me to refactor out all the repeated code. In my real code, it meant that I was able to refactor the methods so they looked like this:

sub all_segments {
    my $self = shift;
    return $self->_gapfill_engine('all');
}

sub all_gapfillers {
    my $self = shift;
    return $self->_gapfill_engine('gap');
}

sub _gapfill_engine {
    my $self = shift;
    my $mode = shift;
    ...
    # foreach and if loops (next stage in refactoring)
    # do one thing if $mode eq 'all'
    # do another thing if $mode eq 'gap'
    return \@segments;
}
[download]

... which is what I was aiming for. Thanks again.

Jim Keenan

My first step would be to reduce the nesting. Each level of if nesting is another and clause the reader of the code has to remember to understand the condition that gets a particular line of code executed.

Often nested ifs can tidy up a lot if you use early exits or negate the condition. Sometimes a redundant test cleans up the nesting sufficiently that the time cost doesn't matter compared to the ease of understanding. Sometimes it's even worth using a exitable if:

while (condition) { # Exitable if
    if (condition) {
        ...
        last; #early exit
    }
   
    ....
    last; # Bad things happen if this if omitted :)
}
[download]

Asside from that you could simply pass a boolen rather than a coderef. Then the test becomes push @foo, [ @bar ] if $doTest and $self->{baz} eq $gamma;.

In addition to some perfectly reasonable ideas already presented, here's the way I do a similar thing. In my case, I'm going to use $baz rather than $self->{baz} to show that it's a lexical. Or, more to the point, that $baz may not be known until you're three levels deep inside _internal_sub and completely dynamic.

sub alpha; # unchanged from your example.
sub beta {
  my $self = shift;
  my $coderef = sub { $_ eq $gamma };
  $self->_internal_sub($coderef);
}

sub _internal_sub {
  my $self = shift;
  my $coderef = shift;
  foreach ...
    if ...
      if ...
        if ...
          local $_ = $baz;
          push @foo, [ @bar ] if $coderef->();
        }
      }
    } else {
      local $_ = $baz;
      push @foo, [ @bar ] if $coderef->();
    }
  }
  local $_ = $baz;
  push @foo, [ @bar ] if $coderef->();
  \@foo;
}
[download]

However, since you have three places you call this coderef, it may be better to just pass in the parameter:

sub beta {
  my $self = shift;
  my $coderef = sub { $_[0] eq $gamma };
  $self->_internal_sub($coderef);
}

# and ...
  push @foo, [ @bar ] if $coderef->($baz);
[download]

1. It's pretty obvious that there's further refactoring that can be done. The number of if-else constructs is CodeSmell and indicates that further refactoring can be done.
2. So long as you maintain the exact same interface and behaviors to the outside world, it doesn't matter how much you have or have not refactored. Basically, this means you should only refactor as much as is necessary for the items you have at the time. Every change is a risk.
3. Only refactor with an automated test suite. Period, end of story.
4. The first refactoring that should be done is variable names. It can always be done to any code, period.
5. If it works, it passes the First Rule.

Hot dog! Would you look at that! Here it is only 10 minutes into the work day and I've already learned my "something new" for the day. Thank you dragonchild for introducing me to the term CodeSmell, which I Super Searched to find your helpful Meditation that defined it for me (though I did a bit of Googling too). For the rest of the day everything else will be gravy.

Is the conditional known before the if-else logic? If so, why not just pass them as a precomputed argument rather than as a closure?

sub alpha {
    my $self = shift;
    my $condition = 1; # always
    return $self->_internal_sub($condition);
}

sub beta {
    my $self = shift;
    my $gamma = get_gamma_somewhere();
    my $condition = $self->{baz} eq $gamma;
    return $self->_internal_sub($condition);
}
sub _internal_sub {
    my $self = shift;
    my $condition = shift;
    ...
    foreach ...
        if ...
            if ...
                if ...
                    push @foo, [ @bar ] if $condition;
                }
            }
        } else {
            push @foo, [ @bar ] if $condition;
        }
    }
    push @foo, [ @bar ] if $condition;
    return \@foo;
}
[download]

xdg wrote:

Is the conditional known before the if-else logic? If so, why not just pass them as a precomputed argument rather than as a closure?

No, it is not. Some of the variables that go into the conditional are more tightly scoped than that or not even declared yet. So I can't pass a pre-computed argument.

jimk

In the absence of more information, I like CountZero's answer, because it's clean and simple.  But xdg's answer is a good one too, especially if there are cases where you might other subroutines (eg. gamma, delta, etc.) to call the new _internal_sub() subroutine.

I did want to point out that, when I encounter a situation like this, it often helps me to ask whether the two subroutines would be likely to evolve together?  That is, if you were to make a change/improvement to alpha(), would you be making the same change/improvement to beta()?  If so, then by all means use a flag (eg. set to "alpha" or "beta") so that you get more mileage from only having to make future changes to one subroutine rather than two.

if a and b and c then do...
if !a then do...

Which, if I'm not messing up my logic analysis, can be simplified to:

if !a or b and c then do...

Both functions should be simplified down to:

sub alphabeta {
    my ($self, $test) = @_;
    ...
    foreach ...
        if (!... || ... && ...) {
            push @foo, [ @bar ] if !$test || $self->{baz} eq $gamma;
        }
    }
    push @foo, [ @bar ] if !$test || $self->{baz} eq $gamma;
    return \@foo;    
}
[download]

I am of course also little confused as to what the second push @foo, [ @bar ] is there for.

my ($alpha, $beta);
$alpha = $beta = << "EOF";
    my $self = shift;
    ...
    foreach ...
        if ...
            if ...
                if ...
                    push @foo, [ @bar ];
                }
            }
        } else {
            push @foo, [ @bar ];
        }
    }
    push @foo, [ @bar ];
    return \@foo;
END
$beta =~ s/(push \@foo, \[ \@bar \]);/$1 if \$self->\{baz\} eq \$gamma
+;/gs;
$beta =~ s/sub alpha/sub beta/;

sub alpha {
  eval $alpha;
}
sub beta {
  eval $beta;
EOF
[download]