Should I avoid nested if statements in Perl?

Question

I'm doing some Perl and seeing my nested "if" statements is driving me mad. I managed to reduce some of them with guard blocks in another section, but I'm stuck here.

Do you think I may leave the code as is, or is there a "proper" way to refactor the following ? (I also admit being relatively new to Perl)

This is actually a subroutine asking for user input on each parameters of a list (external file). $[3] is the matching pattern, $[2] is the default value for the considered parameter (NULL if there is none), $_[1] specifies if it is mandatory or not. the 'next' statement refers to the next parameter read (while loop).

With everyone's help (thanks !), here's the newest version.

100         if ( $input ne '' && ( $input !~ $match || $input =~ /'.+'/ ) ) {
101             print "! Format not respected. Match : /$match/ (without \' \')\n";
102             next;
103         }
104         if ( $input eq '' ) {
105             if ( $default eq 'NULL' ) {
106                 if ( $manda eq 'y' ) {
107                     print "! Mandatory parameter not filled in\n";
108                     next;
109                 }
110                 print "+ Ignoring parameter.\n";
111                 $input = '';
112             }
113             else {
114                 print "+ Using default value\n";
115                 $input = $default;
116             }
117         }

---

 98        if($input eq ''){
 99             if($_[2] eq 'NULL'){
100                 if($_[1] eq 'y'){
101                     print "! Mandatory parameter not filled in\n";
102                     next;
103                 }
104                 else{
105                     print "+ Ignoring parameter.\n";
106                     $input = '';
107                 }
108             }
109             else{
110                 print "+ Using default value\n";
111                 $input = $_[2];
112             }
113         }
114         elsif($input !~ $_[3] || $input =~ /'.+'/){
115                 print "! Format not respected. Match : /$_[3]/ (without \' \')\n"; 
116                 next;
117             }
118         }

Answer 1

Here's a slightly more readable version of chaos' answer:

# Set sane variable names...
my ($is_required, $default, $pattern) = @_

# Convert the external string convention for simpler evaluation...
$default = undef if $default eq 'NULL'

# Refuse problematic input before going any further...
if ($input ne '' && $input !~ $pattern || $input =~ /'.+'/) {
    print "! Format not respected. Match : /$pattern/ (without \' \')\n"; 
    next;
}


# If there was no input string...
if($input eq '') {

    # Set the default, if one was provided...
    if( $default ) {
        print "+ Using default value\n";
        $input = $default;
    } 
    # otherwise, complain if a default was required...
    else {
        if( $is_required eq 'y' ){
            print "! Mandatory parameter not filled in\n";
            next;
        }
        print "+ Ignoring parameter (no input or default provided).\n";
    }
}

The key points are:

• You don't need else if you are exiting the current loop with next
• Exceptional cases should be handled first
• You can greatly improve readability by using named variables

Answer 2

An alternative approach that sometimes helps with readability is to put some or all of the branches in well-named code references. Here is a start on the idea:

$format_not_respected = sub {
    return 0 if ...;
    print "! Format not respected....";
    return 1;
}
$missing_mandatory_param = sub {
    return 0 if ...;
    print "! Mandatory parameter not filled in\n";
    return 1;
}

next if $format_not_respected->();
next if $missing_mandatory_param->();
etc...

Answer 3

if($input ne '' && ($input !~ $_[3] || $input =~ /'.+'/)) {
    print "! Format not respected. Match : /$_[3]/ (without \' \')\n"; 
    next;
}
if($input eq '') {
    if($_[2] eq 'NULL') {
        if($_[1] eq 'y'){
            print "! Mandatory parameter not filled in\n";
            next;
        }
        print "+ Ignoring parameter.\n";
        $input = '';
    } else {
        print "+ Using default value\n";
        $input = $_[2];
    }
}

Answer 4

The main concern is to keep the code readable.

If you can get readable code with nested if statements, go ahead. But keep common sense active at all times.

Answer 5

Common practice is to define constants for your array indexes, and give them meaningful names. Such as:

use constant MANDATORY => 1,
             DEFAULT => 2,
             PATTERN => 3;
...
if($_[DEFAULT] eq 'NULL') {
   ...
}

As far as nesting -- You should often try to reduce the indent (meaning keeping the level of nesting low), but never do so at the expense of keeping the code understandable. I have no problem with the level of nesting, but this is also just a small cut of your code. If it's really an issue, you could break out the conditions into separate subroutines.

Answer 6

If you logic required nested if statement then I guess there is nothing wrong with them.

However, you could improve your code's readability by

1. Using just a little more white space and
2. By using your own variables instead of operating directly on @_

Isn't this a bit more readable?

 98        if ($input eq '') {
 99             if ($default eq 'NULL'){
100                 if ($input eq 'y'){
101                     print "! Mandatory parameter not filled in\n";
102                     next;
103                 }
104                 else {
105                     print "+ Ignoring parameter.\n";
106                     $input = '';
107                 }
108             }
109             else {
110                 print "+ Using default value\n";
111                 $input = $default;
112             }
113         }
114         elsif ($input !~ $foo || $input =~ /'.+'/) {
115                 print "! Format not respected. Match : /$foo/ (without \' \')\n"; 
116                 next;
117             }
118         }

Answer 7

Given that you will probably do a spaghetti goto otherwise, Absolutely No.

What might be better is a switch case.

Answer 8

you could simplify to the following if you don't like all the else's.

if($input eq ''){
    $input = $_[2];
    $output = "+ Using default value\n";
    if($_[2] eq 'NULL'){
        $input = '';
        $output = "+ Ignoring parameter.\n";
        if($_[1] eq 'y'){
            $output = "! Mandatory parameter not filled in\n";
        }
    }
}
elsif($input !~ $_[3] || $input =~ /'.+'/){
    $output = "! Format not respected. Match : /$_[3]/ (without \' \')\n"; 
}
print $output;

Answer 9

I think the main (if not only) reason for concerns regardong nesting is algorithm complexity. On the other cases you should worry about readability and maintainability, which can be adressed with propoer commenting and indentation.

I always find a good exercise in maintanability to read old code of mine, not only to feedback on readability but also on technique...

Answer 10

I suppose you could do logic combinations to flatten it out:

if(($input eq '')&&($_[2] eq 'NULL')&&($_[1] eq 'y')){
  print "! Mandatory parameter not filled in\n";
  next;
}

elsif(($input eq '')&&($_[2] eq 'NULL')){
  print "+ Ignoring parameter.\n";
  $input = '';
}

elsif($input eq ''){
  print "+ Using default value\n";
  $input = $_[2];
  next;
}


elsif($input !~ $_[3] || $input =~ /'.+'/){
  print "! Format not respected. Match : /$_[3]/ (without \' \')\n";
}

print $output;

Then you could use a switch to make it a little better.