|Do you know where your variables are?|
Returning undef: The point I would like Damian to reconsiderby martin (Friar)
|on Jun 22, 2007 at 10:23 UTC||Need Help??|
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.
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.