True + True == 2
I've seen lots of new Perl programmers confuse 1 and true before, usually something like this:
do_something if boolean_function() == 1;
This is not only redundant, but it's also brittle and redundant. In Perl, it's very easy for a boolean function to return something other than one.
sub has_stuff {
...
return scalar @stuff;
}
This code I found today in Module::Build::Base takes the cake.
if ( $self->check_prereq + $self->check_autofeatures != 2) {
UPDATE: Whoops, I broke it! As rsimões pointed out in the comments, the original does not short circuit and my change does. That was not a bug, both check routines should always run (a consequence of the checks having side effects). I had to think about it a bit, and the simplest thing I came up with is:
if( grep { !$_ } $self->check_prereq, $self->check_autofeatures ) {
Which goes to show, and I should have known this myself, don't assume the author is an idiot. There's usually a reason why they did what they did. Find it before touching the code.
Ya know, the guy who tells you to write tests and not use MakeMaker.
Not to mention that doesn't even short-circuit. Oi.
Maybe it's supposed to be that way? The "!= 2" code ensures that both subroutines run, which you may actually want? Although if your "check_*" methods have required side-effects you have a whole other host of problems :-)
Possibly. The only side-effect seems to be output to the user. My question would be why isn't
check_autofeaturesjust part ofcheck_prereq? That would have made more sense in the first place…You're right, it's likely deliberate that it didn't short circuit so the user sees the result of both checks. I'll amend the patch.
A good reason why, if you write something slightly unusual or not immediately obvious, it can be beneficial to leave a brief comment explaining why you did it that way :)
The first thing that came into my mind when I read this: Wow, can't believe the great Schwern fell for that. So another lesson: don't assume a prominent hacker is not an idiot sometimes :) (I'm an idiot too, BTW :).
Also, as a comment on Reddit already noted, this can be made a bit safer by using the double unary trick: if (!!a + !!b == 2) ...
(I'm reminiscing the Turbo Pascal 4/5 days when circuiting behaviour can be set via a compiler option.)
I fall for things like this all the time, because my default mode of operation is not to be fastidious. (As an aside, I'm unable to come up with a positive antonym for fastidious). If I can assume that the code is not being "clever" then I don't have to walk carefully, I can run briskly through the code. Tests will catch me if I trip.
Unfortunately, when changing something "stupid" this is exactly the wrong attitude to have.
Yes, I thought of employing
!!as well. While I think it's safe, I rejected it because I felt it's not a very well known idiom, wasn't sure what it would do with undef and wasn't wholly sure that it's safe. Similar issues with using bitwise operators.