Beefy Boxes and Bandwidth Generously Provided by pair Networks
laziness, impatience, and hubris
 
PerlMonks  

Control Flow - Multiple returns or nested conditionals

by JediWizard (Deacon)
on Oct 07, 2010 at 15:31 UTC ( [id://864019]=perlquestion: print w/replies, xml ) Need Help??

JediWizard has asked for the wisdom of the Perl Monks concerning the following question:

I have had a difference of opinion with a colleague regarding the use of multiple return statements in a sub routine, as opposed to using nested conditionals. I'd like to hear thoughts from the rest of the community on this topic. Please consider the following subroutines: (all method and variables are fake just to illustrate the example)

sub foo { my($self) = @_; my $authentication = $self->authenticate_user(); if(! defined($authentication)){ return {status=>0, error=>"Authentication Failed", external_id=> +undef}; } my $profile = $self->get_user_profile(); if(! $profile->is_active()){ return {status=>0, error=>"User is not active", external_id=>und +ef}; } my($external_id) = $self->find_external_id($profile); if(! defined($external_id)){ $external_id = $self->send_profile_to_partner($profile); } return {status=>1, error=>undef, external_id=>$external_id}; } ### As Opposed to sub bar { my($self) = @_; my $status = 0; my $error = undef; my $external_id = undef; my $authentication = $self->authenticate_user(); if(! defined($authentication)){ $error="Authentication Failed"; }else{ my $profile = $self->get_user_profile(); if(! $profile->is_active()){ $error="User is not active"; }else{ my($external_id) = $self->find_external_id($profile); if(! defined($external_id)){ $external_id = $self->send_profile_to_partner($profile); } } } return {status=>$status, external_id=>$external_id, error=>$error}; }
Corrected a typo causeing bar to always return 0 for status.

The hash reference being returned could be defined at the beginning for either implementation, to make adding or removing values from it easier, my real question is simply regarding the style of using a return to short circuit the sub routine, and opposed to the nested if/else structures.

Which of these implementations would you prefer and why?

Thank you all in advance for sharing your input. I'm interested to hear what people think


They say that time changes things, but you actually have to change them yourself.

—Andy Warhol

Replies are listed 'Best First'.
Re: Control Flow - Multiple returns or nested conditionals
by moritz (Cardinal) on Oct 07, 2010 at 16:17 UTC
    I generally prefer less nesting, and thus direct return. However your example suffers from code duplication for the version that uses return(), though that's not necessary:
    sub err { { status => 0, error => shift(@_), external_id => undef); } sub foo { my($self) = @_; my $authentication = $self->authenticate_user(); return err('Authentication Failed') unless defined $authentication; my $profile = $self->get_user_profile(); return err('User is not active') unless $profile->is_active; my($external_id) = $self->find_external_id($profile); if(! defined($external_id)){ $external_id = $self->send_profile_to_partner($profile); } return {status=>1, error=>undef, external_id=>$external_id}; }
    Perl 6 - links to (nearly) everything that is Perl 6.
Re: Control Flow - Multiple returns or nested conditionals (we're done here)
by tye (Sage) on Oct 07, 2010 at 23:03 UTC

    Early return is probably the single most effective refactoring tool I've found. It is the single technique that has offered the greatest improvements in code clarity and simplicity for me over the last decade.

    I almost always find several cases of "if" + "return" to be much better code than having "elsif"s.

    The second-most-useful tool is throwing exceptions, which is another type of early departure. "next if ..." at the top of a loop is also extremely handy.

    It is very useful to get the small exceptions out of the way up front without having to complicate the lion's share of the code by burying it in nested flow control.

    Compare the simplicity of:

    return ...; vs. my $return; ... $return= ...; # Put some flow control structure here # to prevent any code between here and # the 'return' from running ... return( $return );

    Requiring a single use of the return keyword does no good. It doesn't lead to knowing exactly what the function can return. It just leads to return @computed_above; and then having to go find all of the ways in which @computed_above might be manipulated. So it usually makes the code less clear (and more complicated).

    Multiple exits from a subroutine is not something to avoid. Quite the opposite.

    - tye        

      Well that's exactly what I said… except, you know, better, and with illustrative examples, and a firm conclusion, and stuff.

Re: Control Flow - Multiple returns or nested conditionals
by superfrink (Curate) on Oct 07, 2010 at 16:07 UTC
    In general I lean towards less nesting. Less nesting makes the code easier to map into a model in my head. A lot of nesting in one place make me question if there should be a subroutine.

    I have seen code with CLAIM comments that signal to the reader that something is true at that point in the program. (Well, that the programmer intended it to be and believes it to be true.) See the following example.
    sub foo { my($self) = @_; my $authentication = $self->authenticate_user(); if(! defined($authentication)){ return {status=>0, error=>"Authentication Failed", external_id=> +undef}; } # CLAIM : the user is authenticated my $profile = $self->get_user_profile(); if(! $profile->is_active()){ return {status=>0, error=>"User is not active", external_id=>und +ef}; } # CLAIM : the user profile is active my($external_id) = $self->find_external_id($profile); if(! defined($external_id)){ $external_id = $self->send_profile_to_partner($profile); } # CLAIM : the user profile has been sent to the partner return {status=>1, error=>undef, external_id=>$external_id}; }
Re: Control Flow - Multiple returns or nested conditionals
by jethro (Monsignor) on Oct 07, 2010 at 16:20 UTC

    Not using multiple returns is one of those old rules that emerged out of the 'Goto considered harmful' war, I think. Still the goal to reach is always to have the most readable and maintainable code. The danger of multiple returns is that maintaining it can be difficult. So if that danger is more than compensated by a clearer structure of the sub, it is acceptable IMHO.

    I can think of two general types of subroutine where that is often the case:

    1) When the logic of a subroutine has many exceptions or error conditions.

    2) The "big switch" subroutine, i.e. the subroutine with a long switch/case statement

    When I use multiple returns in a subroutine, I always mention it prominently in the comments at the head of the sub.

    Whether your example is already exceptional enough for multiple returns is a matter of taste. I think I wouldn't have used them there, but I might have given a different answer yesterday ;-).

Re: Control Flow - Multiple returns or nested conditionals
by BrowserUk (Patriarch) on Oct 07, 2010 at 16:20 UTC

    Assuming the first example is the correct one as far as the status value goes, I'd code it this way.

    Update:tweaked it a bit.

    sub bar { my($self) = @_; my $rv = { status => 0, error => undef, external_id => undef }; $rv->{ error } = "Authentication Failed" and return $rv unless $self->authenticate_user(); my $profile = $self->get_user_profile(); $rv->{ error } = "User is not active" and return $rv unless $profile->is_active(); $rv->{ external_id } = $self->find_external_id( $profile ) // $self->send_profile_to_partner( $profile ); $rv->{ status } = 1; return $rv; }
Re: Control Flow - Multiple returns or nested conditionals
by TomDLux (Vicar) on Oct 07, 2010 at 20:48 UTC

    I very much like early exits ( return, next,last ) when they avoid getting deep into stuff, especially dealing with exceptions, errors, irregularities. This example allows empty lines and comments in a data file.

    open my $datafile ... LINE: while ( my $line = <$file> ) { next LINE unless length $line; next LINE if ( $line =~ m{\A \s* #}xms; process $line; }

    If you're selecting one of several possible return values, I think it better to determine the value and return it in a single place. Otherwise you have a situation where a return value of XYZ may have arisen from this return statement or from that return statement. If the blocks are more than a few lines long, it may be worth encapsulating it into a named subroutine. That way the main block is short and easy to read:

    if ( option_A( $line ) ) { $retval = do_this_complicated_thing( $line ); } elsif ( option_B( $line ) { retval = 'alphagetti'; retval .= ' and maple syrup' if $option_C( $line ); } else { $retval = 42; } return $retval;

    As Occam said: Entia non sunt multiplicanda praeter necessitatem.

Re: Control Flow - Multiple returns or nested conditionals
by Your Mother (Archbishop) on Oct 07, 2010 at 17:37 UTC

    I'd argue for whichever structure feels the most natural for the given use. I find that I have to twist or contrive temporary variables and such more often when attempting to avoid multiple return statements so I tend to prefer the multiple style. As others have said, it is often easier to read and avoiding long or deep nesting almost always improves readability.

Re: Control Flow - Multiple returns or nested conditionals
by eyepopslikeamosquito (Archbishop) on Oct 07, 2010 at 20:50 UTC

    I find the first one clearer and easier to understand.

    Our coding standards do not mandate "single point of exit"; by all means use it where it makes the code clearer, but enforcing it leads to clumsy, deeply nested code, with extra "flag" variables to keep state. At least that's what I've seen. In any case, with the growing use of exceptions in modern languages, single point of exit is somewhat illusory.

    More important than single point of exit is to keep functions cohesive and short, with few parameters, and free of globals.

Re: Control Flow - Multiple returns or nested conditionals
by halfcountplus (Hermit) on Oct 07, 2010 at 15:53 UTC

    I don't see anything wrong with either one, altho I tend toward the first one. (In fact, what I might do would be to define a populated return hash first, then modify it as necessary -- kind of like the 2nd example -- but within the if...return structure of the first example). If I had to defend this, I'd point out:

    • It's simpler and perhaps more clear.
    • It's probably more efficient.
Re: Control Flow - Multiple returns or nested conditionals
by lostjimmy (Chaplain) on Oct 07, 2010 at 16:03 UTC
    My company's coding standard requires a single return at the end of each function/method. I think the idea behind that is that you always know where the exit from the function occurs. What you end up with is a significant increase in conditional statements and lots of nesting, as your examples show. I'm not sure whether or not I agree or disagree with that approach, but it's what I've gotten used to.
      I think the idea behind that is that you always know where the exit from the function occurs.
      The idea comes from "structured programming". Dijkstra's famous "Go To Considered Harmful" isn't about avoiding the goto keyword. It's about having structured units in your program (what we now call blocks), each with a single entry and exit point. Goto defeats that, but next, last, redo, and multiple return statements do so as well.

      Structured programming is considered useful is when you are make statements (proofs) about the program itself. Proofs typically make use of pre- and post conditions. Not having single entry/exit points make those proofs much harder, if not impossible.

Re: Control Flow - Multiple returns or nested conditionals
by Marshall (Canon) on Oct 08, 2010 at 01:46 UTC
    I believe that the first example is better than then 2nd.
    Why? Because it is easier to understand. It wins hands down - with no doubt at all in my mind - but obviously you have some doubts.

    Clarity should be the key thing, not conformance to some arbitrary "coding style rule".

    The "rules" about things like "no multiple returns" have to be taken in the context of the "art of programming". Judgment is required.

    Option #1 has 10 program statements aside from boiler-plate, 3 of which have a return value or about 1/3 of the statements send something back to the caller (produce a result).

    The arguments against having multiple return statements are the same as the ones against the "go to". But this is a bogus argument if you have a function that is expressed in some dozen lines or say 1/2 page and it has a very clear purpose and a very clear return value.

    The "go to" is deadly when it jumps to widely separated parts of the code. That's not what we have here!

    I just don't buy the argument that you are going to modify a 10 line (or even 1/2 page) subroutine without understanding what those 10 lines do!

    Calling a sub that has multiple return points can simply the overall code's logic and increase the maintainability of the code. Reducing the level of indenting is a huge factor in clarity. Clarity is a good thing, not a bad thing.

      Actually Marshall, I agree with you whole heartedly. I was quite deliberate in my asking of this question to not state up front which of the implementations I prefer, so that the responses I got would not be influenced by my opinion. It has been my experience that when asking this type of question, the best way to get clear unbiased responses is to remove as much of my subjective position from the description as possible...

      That having been said, at this point it is quite clear from the responses in this thread, that my preference for multiple returns is the more popular opinion. That may not do me any good in my disagreement with my colleague, but it does hold some level of personal satisfaction.


      They say that time changes things, but you actually have to change them yourself.

      —Andy Warhol

        I posted some code earlier today at Re: Checking contents of array against contents of a hash. There is a subroutine, is_array_subset() which contains 2 return() statements. I am curious as to how your colleague would code this sub?

        I used less than 10 lines of code (I don't count curly braces or blank lines as lines of code). I actually added the "@extra" feature after doing the main part - and having multiple returns made that easier to do.

Re: Control Flow - Multiple returns or nested conditionals
by jakeease (Friar) on Oct 08, 2010 at 10:08 UTC

    I tend to prefer the multiple returns. Nothing like modifying someone else's 1500 line foreach loop filled with conditionals nested four deep slinging hashes, or HoHoAoHoHoHoHoAoH's 9 levels deep to make you yearn for an exit or two. :-)

    from Refactoring: Improving The Design Of Existing Code by Martin Fowler.

    Using "Replace Nested Conditional With Guard Clauses" refactoring method.

    Original Code

    double getPayAmount() { double result; if(m_isDead) result = deadAmount(); else { if(m_isSeparated) result = separateAmount(); else { if(m_isRetired) result = retireAmount(); else result = normalPayAmount(); }; } return result; }

    Refactored Code

    double getPayAmount() { if(m_isDead) return deadAmount(); if(m_isSeparated) return separatedAmount(); if(m_isRetired) return retiredAmount(); return normalPayAmount(); }
      Please, if you're posting exemplar code, fix the indenting before you post. Even if it's screwed up by copying and pasting, there's still the spacebar.
Re: Control Flow - Multiple returns or nested conditionals (refactor)
by tye (Sage) on Oct 08, 2010 at 05:06 UTC

    Looking closely at the specific example you gave, I see some repeated work at each return that would give your coworker more reason to fight against multiple returns. I'd probably just factor that out:

    sub _foo { my( $self )= @_; my $authentication= $self->authenticate_user(); return "Authentication Failed" if ! defined $authentication; my $profile= $self->get_user_profile(); return "User is not active" if ! $profile->is_active(); my $external_id= ( $self->find_external_id($profile) )[0] // $self->send_profile_to_partner( $profile ); return( '', $external_id ); } sub foo { my( $self )= @_; my( $error, $external_id )= $self->_foo(); return { status=>0, error=>$error, external_id=>undef } if $error; return { status=>1, error=>undef, external_id=>$external_id }; }

    I also question the wisdom of an interface that separately returns 'status' and 'error'. If 'error' is false (including undefined) then the call was a success. An even better interface in the long run is to throw exceptions in the case of failures.

    - tye        

Re: Control Flow - Multiple returns or nested conditionals
by BrowserUk (Patriarch) on Oct 07, 2010 at 16:02 UTC

    The two aren't the same. The second always return status = 1 regardless of whether an error has occured.

Re: Control Flow - Multiple returns or nested conditionals
by GotToBTru (Prior) on Oct 07, 2010 at 21:41 UTC
    My preference is the nested conditionals. It encourages and facilitates reviewing the entire logical flow if/when you add other conditions. With multiple return points, if the code was modified extensively over time, I would worry about ending up with orphaned parts of the code.
Re: Control Flow - Multiple returns or nested conditionals
by RecursionBane (Beadle) on Oct 08, 2010 at 20:21 UTC

    Thanks for creating this node. I've often wondered if what I'm doing is the right thing. I tend to flip-flop between both methods based on the following criteria:

    * If the routine has flow modifiers like goto or next, I use a single return statement.

    * If the routine has no flow modifiers and has code duplication that aids readability, I use multiple return statements to keep each duplicated section independent (they can now be jettisoned on-demand).

    * If the routine has flow modifiers and code duplication, I use multiple return statements with a debug print statement before every return (during development). I also suddenly turn religious and start praying.

    Writing mostly baby Perl (see "Baby" Perl versus "Bad" Perl), though, means I don't really adhere to "coding standards".

    ~RecursionBane

Re: Control Flow - Multiple returns or nested conditionals
by FloydATC (Deacon) on Oct 08, 2010 at 21:32 UTC
    For complex logic, I always use a single exit point to ease code maintenance. When the code logic dictates different types of returns depending on the circumstances I try to use some sort of state machine and put my exit points at the bottom so atleast I don't have to hunt them down later.

    Deeply nested unless..else statements with a few negated or clauses and recursive sub routine calls can be enough fun without the added confusion of multiple exit points :-)

    For a sequence of tests like in your post though, there's no reason to perform any more processing if one test fails so return the status/error/result/whatever and get out of there. IMHO.

    -- Time flies when you don't know what you're doing

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://864019]
Approved by marto
Front-paged by ww
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others sharing their wisdom with the Monastery: (4)
As of 2024-03-29 12:23 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found