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


in reply to Re^5: inheritance: constructors
in thread inheritance: constructors

I don't buy the argument that "unless" "reads better" than "if !" (nor that "reads better" is always a good criteria for evaluating code, especially not "reads more like English prose").

I do buy the argument that making the negation more subtle (by using "unless" instead of "if !") also means that you increase the odds of mentally dropping the negation and either introducing a bug or wasting too much time being confused (having experienced this and having seen others do it and having seen quite a few bugs make it to Production that were introduced via 'unless').

Another unfortunate side-effect of that "subtle 'not'" that I was a bit surprised and dismayed to repeatedly see in real code recently, was constructs like:

unless( $a ne $b ) {

and worse. The 'not' is subtle so once you start using 'unless' all over the place you'll likely start introducing double negatives.

So I have a strong preference for:

if( $a eq $b ) { # or if( $a ne $b ) {

over anything using 'unless'.

Which brings me to a subject we were discussing this week at work. I've heard (or read) a lot of people agreeing that "of course" you should not use 'unless' with complex logical expressions. But that seems to me to be the one place where I can see 'unless' having some value:

croak( "You must pass foo() a non-empty array ref" ) unless $arg && 'ARRAY' eq ref($arg) && 0 < @$arg;

Directly turning that 'unless' into an 'if' gives us:

croak( "You must pass foo() a non-empty array ref" ) if ! $arg || 'ARRAY' ne ref($arg) || 0 == @$arg;

Now, I don't personally find that particular 'if' obviously worse than the prior 'unless'. But I acknowledge that it is more complex and can see it being harder or slower to parse for many. So I'm somewhat reluctant to make such a transformation when refactoring code.

And it can get "worse":

croak( "You must pass foo() a non-empty array ref or hash ref" ) unless 'ARRAY' eq ref($arg) && 0 < @$arg || 'HASH' eq ref($arg) && 0 < keys %$arg;

This would require adding parens, which can reduce readability considerably (not terrible in this example, though -- certainly more complex):

croak( "You must pass foo() a non-empty array ref or hash ref" ) if( ( 'ARRAY' ne ref($arg) || 0 == @$arg ) && ( 'HASH' ne ref($arg) || 0 == keys %$arg ) );

So, I'm more of the opinion that 'unless' should only be used in the case of complex logical expressions. That is, only for complex logical expressions where the 'unless' version is significantly simpler than the 'if' version.

For the sake of completeness, I'll note that I often find value in removing the complex logical expression (which also removes any benefit to using 'unless', IMO). In the above example, if we get an error about "You must pass foo() a non-empty array ref or hash ref", then we have several possible cases to consider and this could add a step or two to debugging that might add significant time to the process. So it might be better more like:

croak( "You didn't pass foo() a reference" ) if ! $arg || ! ref $arg; if( 'ARRAY' eq ref $arg ) { croak( "You passed foo() an ARRAY ref but it was empty" ) if 0 == @$arg; ... } elsif( 'HASH' eq ref $arg ) { croak( "You passed foo() a HASH ref but it was empty" ) if 0 == keys %$arg; ... } else { croak( "You passed foo() a ", ref($arg), " ref (must be ARRAY or HASH)", ); }

That looks a lot more complex but in reality, it will likely eliminate duplication because your check for "is an ARRAY ref" needs to also be done where you decide what to actually do with the ref and this structure lets you do that check just once. That can also have benefits by keeping the error checking closer to the associated code so that deciding to handle an empty hash is more likely to have the error checking updated to match at the same time.

I'm now considering if I should avoid the subtle 'not' in complex cases by using the low-precendence 'not' as in 'if not ...' to replace the 'unless'. I haven't tried that anywhere yet, so I can't really recommend it yet.

Note that I've seen experienced programmers introduce real bugs that make it to Production because they used 'and', 'or', and/or 'not' in logical expressions (presumably because it "reads better") and were not always aware enough of some of the impacts of the extremely low precedence. So "if( not ... )" would end up being a special-case exception to our rule against using low-precedence logical operators in logical expressions.

- tye