Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine

Re^5: inheritance: constructors

by Athanasius (Chancellor)
on Dec 01, 2012 at 04:41 UTC ( #1006571=note: print w/replies, xml ) Need Help??

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

Well, in

if (...) { die ...; } else { # continue with the happy path }

the provision of an else block is redundant, since it suggests (misleadingly) that the code can somehow die and yet still resume execution following the else — which it can’t.

For this case, I would prefer:

unless ($value == $expected_value) { $errorcount++; die new Exception msg => "argh!"; } # continue with the happy path

which reads more naturally (to my ear). (Note that the original objection was not to unless, but to unless ... else.)

Update: Nice use of the <abbr> tag, BTW — I’ve learnt something new, could come in handy.  :-)

Athanasius <°(((><contra mundum

Replies are listed 'Best First'.
Re^6: inheritance: constructors (unless)
by tye (Sage) on Dec 03, 2012 at 15:41 UTC

    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        

Re^6: inheritance: constructors
by jdporter (Canon) on Dec 01, 2012 at 15:51 UTC

    Right. Which means your "rule" is inconsistent. What happens inside any of the blocks shouldn't matter.

Re^6: inheritance: constructors
by pemungkah (Priest) on Jun 19, 2013 at 23:05 UTC
    I would, in this case, be tempted to write
    die if ...; #happy path continues
    I might even be willing to accept a die unless if the positive version is easier to read than the negative one.

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://1006571]
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others musing on the Monastery: (9)
As of 2018-06-22 11:59 GMT
Find Nodes?
    Voting Booth?
    Should cpanminus be part of the standard Perl release?

    Results (124 votes). Check out past polls.