http://www.perlmonks.org?node_id=622766

I like TheDamian's book Perl Best Practices a lot. All of its guidelines are sound advice and well enough explained that you can grasp the concepts behind them and improve your general code quality awareness along the way.

Which means, this book is something to be tackled with and not just some random legislature imposed on us by way of Someone's impeccable resolution. It also means I have to come up with factual arguments where I disagree.

One PBP rule I am indeed not happy with is this:

Use a bare return to return failure. (Page 199)

The motivation for this guideline examines a subroutine that is meant to return different things in list versus scalar context: a list of variable length of values versus a single average value.

For such a subroutine, the reasoning continues, signaling failure by returning undef would be a particularly poor choice, since in list context this would not amount to false. On the other hand, an empty return statement neatly evaluates to an empty list in list context and undef in scalar context. In turn, checking the boolean value of an assignment of the function results would evaluate to false in both contexts.

To me, this is all true but horribly contrived. With such a principal witness one could put the pope in jail, in a manner of speaking.

First of all, doing different things in different contexts is poor interface design. We see this happen, of course, but far too often.

Secondly, signaling failure in a way that is not clearly out of bounds of other results is another grave mistake. The length of the array of result values in this example has not been established. From what we see, we can neither be sure that it won't be empty in a successful run -- especially in the light of it being empty by default and $failed being false by default -- nor that the formula supposed to yield an average value is saveguarded against division by zero.

On the caller's side, the boolean value of the assignment is of questionable significance, too, because we are dealing with numeric values that can of course be defined but false, or lists that might be empty.

To summarize, we have a very convincing negative example, but changing its return statement by no means strikes me as the key to make it much better.

We could come up with other examples, no doubt, where false is out of bounds, as is an empty list, as is undef. That might be a function supposed to return an object or a fixed positive number of values on success, say. However, I stand by my opinion that these cases should not be treated as showcases for empty return statements.

On the other hand, a single-value returning function should indeed explicitly return a single value in almost any circumstances. That of course includes cases where an undefined value is conveying some information, even if that sounds like a mild form of failure. If the function is advertised as returning foo in this case and undef in that case, I would argue whether returning undef has failure quality at all.

An empty return statement would be a mistake there, precisely because the function might be called in list context. In Perl, list context is sometimes inconvenient to avoid, for example if an expression happens to be used as a subroutine argument. We should not expect our undef function result to vanish and be replaced by the next argument in such a situation, I think.

Example:

sub foo { return; } sub bar { my ($a, $b, $c) = @_; return if !defined $b; die if !defined $c; } bar(1, foo(), 3); # explodes

However, the guideline on page 199 is bound to be misunderstood in a way that return undef be considered a Bad Thing in and by itself. The most prominent supporters of that notion are the authors of Perl::Critic -- another finely crafted tome of wisdom, by the way -- who flag every occurrence of such a return-statement as a breach of policy of highest severity, by default.

I suggest that this guideline should be dropped, as it tends to be misleading.

We might say, don't expect your users to know that a list assignment of one undef value evaluates to true in boolean context, alright. Or, maybe, don't return undef when you mean an empty list. Better still, I would like:

Don't change subroutine results based on list or scalar context.

The example can stay.

Replies are listed 'Best First'.
Re: Returning undef: The point I would like Damian to reconsider
by shmem (Chancellor) on Jun 22, 2007 at 12:43 UTC
    Don't change subroutine results based on list or scalar context.

    Why? I have useful subroutines that produce data and behave different in void, scalar and list context:

    $thingy -> foo (1,2); # results printed to current filehandl +e $string = $thingy -> foo (1,2); # results returned as a single string @lines = $thingy -> foo (1,2); # results returned as a bunch of lines

    and I don't see that as a bad practice. Works like expected and is documented. The alternative would be to shove the result explicitly through three other subs:

    $thingy -> foo (1,2) -> to_output_channel; $string = $thingy -> foo (1,2) -> as_string; @lines = $thingy -> foo (1,2) -> as_list;

    which gains me what exactly? Perl is context aware, even builtins do change their result depending on context, eg localtime. Why should I avoid perl's dwimmery? Should I avoid localtime and roll my own?

    update: I have PBP on my shelf as well, and my biggest critic to it is about its title. It should have been named "Damian Conway's Coding Style", or "Perl Pitfalls" subtitle "and how to avoid them", or even "Code Perl Robustly" subtitle "without knowing all of perl".

    My biggest critic upon Perl::Critic is that it enforces PBP's bad title as a standard, and where it could help you it doesn't, and will never: it can't tell whether you know what you are doing or have fallen into a trap, since it can't read your mind.

    update 2: Personally I think that the rule "Use a bare return to return failure" cannot be a rule, since its text omits context. It's just bad worded and should be:

    To return failure, use a bare return in list context, undef in scalar or void context. Set $@ to something meaningful.
    Perl::Critic should handle the rule that way.

    --shmem

    _($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                                  /\_¯/(q    /
    ----------------------------  \__(m.====·.(_("always off the crowd"))."·
    ");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}
      I ++'d your post but I disagree about your comments on the title of PBP. The book is full of well thought out recommendations that have been vetted by several members of the Perl community so I really do not see a problem with the name. Yes there will be some cargo cult type followers and yes, incompetent management will want to use it as a style guide. But really, given the choice between PBP and an in-house standard cobbled together by said incompetent management which would you prefer!? Overall I think the Perl community has needed something like PBP for a long time and the benefits of its existence far outweigh the negatives (including the name).
        I disagree about your comments on the title of PBP.
        ...
        the benefits of its existence far outweigh the negatives (including the name).

        Don't you see a contradiction here? So far you have defended the book, and I agree. You did not, though, explain why you disagree with me saying the title is bad - you just say it doesn't matter.

        Well, for me, it matters.

        But really, given the choice between PBP and an in-house standard cobbled together by said incompetent management which would you prefer!?
        What choice is that? Given the choice between a burger from <insert-junk-food-company-here> and your own fried shoe soles without salad and no onion, what would you prefer?

        I would see this most excellent book from Damian Comway as a style guide or vade mecum to gain perl insight and develop a good inhouse coding standard, rather than having the book as a whole imposed as such, without reading it, and merely because of its title.

        Which means: if anybody, including the PHB, after reading it gets enlightened and shouts "hey, these are really jolly well best perl practices!" and goes to introduce a subset or all as standard, that's perfectly fine.
        It is not ok if someone comes and says "here, have Perl Best Practices. It's standard now." - "Why?" - "Because they are. Can't read the title?"

        --shmem

        _($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                                      /\_¯/(q    /
        ----------------------------  \__(m.====·.(_("always off the crowd"))."·
        ");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}
      and I don't see that as a bad practice. Works like expected and is documented. The alternative would be to shove the result explicitly through three other subs:
      $thingy -> foo (1,2) -> to_output_channel; $string = $thingy -> foo (1,2) -> as_string; @lines = $thingy -> foo (1,2) -> as_list;
      which gains me what exactly?
      It gains you
      1. explicit clarity of intent
      2. 3 parameterizable output methods
      3. 3 methods which can be re-used or overloaded as needed
      4. The ability to call all 3 output methods, none or any combination simply by mixing/matching method calls.
      Should I avoid localtime and roll my own?
      Well, no. Try Date::localtime or Time::Local or anything in the DateTime hierarchy.


      Carter's compass: I know I'm on the right track when by deleting something, I'm adding functionality
        1. explicit clarity of intent

        I have that on the LHS.

        2. 3 parameterizable output methods
        3. 3 methods which can be re-used or overloaded as needed
        4. The ability to call all 3 output methods, none or any combination simply by mixing/matching method calls.

        No call for those, since that doesn't gain me anything.

        You missed my point. Context awareness is a key concept of Perl. Deprecating context awareness with regard to subroutine return is tantamount to crippling the language.

        Try Date::localtime

        No such module... ;-)

        --shmem

        _($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                                      /\_¯/(q    /
        ----------------------------  \__(m.====·.(_("always off the crowd"))."·
        ");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}
Re: Returning undef: The point I would like Damian to reconsider
by jettero (Monsignor) on Jun 22, 2007 at 10:43 UTC
    I posted a similar sentiment back in January of this year. My comments aren't totally worth mentioning, but Tye and Chromatic have a rather fascinating argument about it that's worth reading [596717]. There are about thirty other nice posts under there too, many of which I haven't even seen before.

    -Paul

Re: Returning undef: The point I would like Damian to reconsider
by merlyn (Sage) on Jun 22, 2007 at 21:06 UTC
    Don't change subroutine results based on list or scalar context.
    If I were trying to follow that advice, what should I do if a subroutine that returns a list in a list context gets called instead in a scalar or void context? "die"?

    Just trying to think this through.

    Or did you actually mean "if your subroutine naturally returns a scalar, be sure to return that same scalar in a list context unless otherwise documented" or something convoluted like that?

    And the timing for this couldn't be better... I'm just now putting the finishing touches on the Perl Best Practices full-day course that I'm presenting at YAPC::NA next week, and have had my own thoughts on many issues such as these during the writing of the course.

      In the past, when faced with this question, I've usually gone on the principle that if a function normally/ever gets called in list context, then it should always return a "list", even in scalar context, i.e. if called in scalar context return the last value in its return expression. This, imho, is the minimally surprising behavior in most scenarios, and is also easiest to code as you don't have to code for sensitivity to calling context. OTOH, obviously there may be times when you'd rather have some other behavior; the only other one you should "reasonably" implement is having it act like an array, i.e. return the return list length when called in scalar context. But even that would warrant documentation including several exclamation points. Any other behaviors should, I think, be limited strictly to internal function calls, not in a public interface.

      A word spoken in Mind will reach its own level, in the objective world, by its own weight
        then it should always return a "list", even in scalar context, i.e. if called in scalar context return the last value in its return expression.
        Ugh, that's not possible. You cannot return a list in a scalar context. Ever.

        You can type:

        return (99, 100, 101);
        But that's not "returning a list in a scalar context", even if called in a scalar context. It's returning a scalar in a scalar context, by evaluating that comma-expression in a scalar context, getting the result 101.

        I know you know that, but sloppy language here gets us running around in circles.

        See my On Scalar Context for samples of many things that "listy" things do in a scalar context. Based on that, I don't see how "last thing" is any more intuitive than "first thing" or "number of elements" (the three main candidates).

      Don't change subroutine results based on list or scalar context.

      If I were trying to follow that advice, what should I do if a subroutine that returns a list in a list context gets called instead in a scalar or void context? "die"?

      Coercing a strictly list returning function into returning a scalar would be wrong and could be treated as an error of the caller if detected.

      But you caught me there, the subroutine would still need to be context-aware to detect that situation. The impact on the subroutine code could be kept at bay, though, if we tucked that piece of logic away, say, in a listonly convenience function. Is something like that already on CPAN? Hmmmm...

        use Contextual::Return; use Carp qw(croak); ... sub foo { ... return LIST { some listy expression here } SCALAR { croak "please invoke me in a list context" } ; }
Re: Returning undef: The point I would like Damian to reconsider
by CountZero (Bishop) on Jun 23, 2007 at 08:55 UTC
    I think "The One Rule" is that you must clearly document the return values of your methods or subroutines and expect the users to read the docs.

    CountZero

    A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James

      Hmm... would an all-encompassing "One Rule" be called the "One Rule to Ring Them All"...?
      :-)
Re: Returning undef: The point I would like Damian to reconsider
by whereiskurt (Friar) on Jun 22, 2007 at 14:41 UTC

    I love perlmonks because it constantly reaffirms that there's always more than one way to do it. When you have a question, there's usually more than one 'right' answer. The question is: when I call some encapsulated piece of code, how will I know whether it succeeded or failed? Was there an error and what should I do?

    The reason why I like the PBP 'rule' is because it's an easy to remember answer to a question that should be considered on every function call. Defensive programming means checking return values. The concept of what to return on failure is like a mini-protocol. "Use a bare return to return failure" is such an easy protocol....

    For all the folks who think it's stupid, and hate the idea of typing 'return 1;', make up your own mini-protocol and a call it Perl's SecondBest Practices. LOL :-)

    "Don't return undef when you mean an empty list" -- this is some serious wisdom, too. I'm really glad you said that. :-) I think it's worth mentioning that PBP also recommends using Contextual::Return to deal with that exact 'complication'.

    IMO The Best Practice here is a combination of using "return;" anywhere something goes wrong and at the bottom of your subs using a block like this:

    use Contextual::Return; sub items { ## Something failed! if ($failure) return; #TODO: Consider croaking! ## Being explicit! return ( LIST { qq(Item1 Item2) } HASHREF { {Item1 => 'big', Item2 => 'small'}} SCALAR { 2 } ); }

    I think knowledge sharing and promoting 'the right way' is what will allow the Perl Community to continue to flourish. I think Damian was Noble in pursuing this ideal.

    Kurt

    Updated: Added points about Contextual::Return

Re: Returning undef: The point I would like Damian to reconsider
by chromatic (Archbishop) on Jun 22, 2007 at 17:37 UTC
    An empty return statement would be a mistake there, precisely because the function might be called in list context. In Perl, list context is sometimes inconvenient to avoid, for example if an expression happens to be used as a subroutine argument. We should not expect our undef function result to vanish and be replaced by the next argument in such a situation, I think.

    The only time where this would harm the code I tend to write is if I had a semi-predicate problem in a function that returned a list. (The only reason I don't run into the problem where hash declarations impose list context is that I don't do much CGI programming, where I might call $q->param( 'foo' ) in a hash assignment.)

Re: Returning undef: The point I would like Damian to reconsider
by Raster Burn (Beadle) on Jun 22, 2007 at 16:05 UTC
    I think the truly "best" practice would be to use an exception class hierarchy and throw exceptions upon errors. I <3 Exception::Class
      I think the truly "best" practice would be to use an exception class hierarchy and throw exceptions upon errors. I <3 Exception::Class
      Agreed, but that was not the point of this guideline. To be fair to TheDamian, he dedicated a whole chapter to error handling, where among other things he discusses exception hierarchies in depth.

      Damian's point in this guideline was, one should use an empty return statement where returning undef could lead to unexpected results, while my main point was that he failed to properly distinguish that exotic case from good uses of returning undef, seeing that now some programmers think it should be avoided altogether.

      Personally, I prefer to use empty return statements only for void context, i.e. subroutines or methods that don't offer a return value. If I found myself catering for context-aware return value polymorphism I would not mind to use a more explicit construct.