CODEWITHSUNDEEP
perlcoding-stylenested
Isaac Clarke
Isaac Clarke

Reputation: 727

Should I avoid nested if statements in Perl?

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         }

Upvotes: 5

Views: 6623

Answers (10)

Shawn J. Goff
Shawn J. Goff

Reputation: 4765

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.

Upvotes: -2

FMc
FMc

Reputation: 42421

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...

Upvotes: 4

Donato Azevedo
Donato Azevedo

Reputation: 1418

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...

Upvotes: 0

ire_and_curses
ire_and_curses

Reputation: 70250

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

Upvotes: 10

chaos
chaos

Reputation: 124365

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

Upvotes: 3

innaM
innaM

Reputation: 47899

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         }

Upvotes: 1

Toon Krijthe
Toon Krijthe

Reputation: 53476

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.

Upvotes: 3

Mark Synowiec
Mark Synowiec

Reputation: 5445

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;

Upvotes: 0

nik
nik

Reputation: 13468

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

What might be better is a switch case.

Upvotes: 0

Andrew Barnett
Andrew Barnett

Reputation: 5174

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.

Upvotes: 2

Related Questions