views:

84

answers:

6

I remember many years back, when I was in school, one of my computer science teachers taught us that it was better to check for 'trueness' or 'equality' of a condition and not the negative stuff like 'inequality'.

Let me elaborate - If a piece of conditional code can be written by checking whether an expression is true or false, we should check the 'trueness'.

Example: Finding out whether a number is odd - it can be done in two ways:

if ( num % 2 != 0 )
{
  // Number is odd
}

or

if ( num % 2 == 1 )
{
  // Number is odd
}

(Please refer to the marked answer for a better example.)

When I was beginning to code, I knew that num % 2 == 0 implies the number is even, so I just put a ! there to check if it is odd. But he was like 'Don't check NOT conditions. Have the practice of checking the 'trueness' or 'equality' of conditions whenever possible.' And he recommended that I use the second piece of code.

I am not for or against either but I just wanted to know - what difference does it make? Please don't reply 'Technically the output will be the same' - we ALL know that. Is it a general programming practice or is it his own programming practice that he is preaching to others?

NOTE: I used C#/C++ style syntax for no reason. My question is equally applicable when using the IsNot, <> operators in VB etc. So readability of the '!' operator is just one of the issues. Not THE issue.

A: 

I remember hearing the same thing in my classes as well. I think it's more important to always use the more intuitive comparison, rather than always checking for the positive condition.

BenV
A: 

Really a very in-consequential issue. However, one negative to checking in this sense is that it only works for binary comparisons. If you were for example checking some property of a ternary numerical system you would be limited.

Kevin Sylvestre
+1  A: 

I've had people tell me that it's to do with how "visible" the ping (!) character is when skim reading.

If someone habitually "skim reads" code - perhaps because they feel their regular reading speed is too slow - then the ! can be easily missed, giving them a critical mis-understanding of the code.

On the other hand, if a someone actually reads all of the code all of the time, then there is no issue.

Two very good developers I've worked with (and respect highily) will each write == false instead of using ! for similar reasons.

The key factor in my mind is less to do with what works for you (or me!), and more with what works for the guy maintaining the code. If the code is never going to be seen or maintained by anyone else, follow your personal whim; if the code needs to be maintained by others, better to steer more towards the middle of the road. A minor (trivial!) compromise on your part now, might save someone else a week of debugging later on.

Update: On further consideration, I would suggest factoring out the condition as a separate predicate function would give still greater maintainability:

if (isOdd(num))
{
    // Number is odd
}
Bevan
Hey yeah! I've missed quite a few `!` myself when reading someone else's code. Especially if the code is like `if(!(some-condition))`. It didn't occur to me! :D
Senthil
About your update - I don't exactly know what a predicate means, but I am guessing the isOdd() function will contain the condition, wouldn't it?
Senthil
+1 For the predicate function... I do that constantly and it greatly improves readability imho.
Helper Method
"Predicate" has multiple meanings (see http://www.google.co.nz/search?q=define:predicate) but in this case it's a fancy name for a function that tests a condition and returns true or false. Sorry for the Jargon! (though it seems to be an occupational hazard ...)
Bevan
+2  A: 

I will disagree with your old professor - checking for a NOT condition is fine as long as you are checking for a specific NOT condition. It actually meets his criteria: you would be checking that it is TRUE that a value is NOT something.

I grok what he means though - mostly the true condition(s) will be orders of magnitude smaller in quantity than the NOT conditions, therefore easier to test for as you are checking a smaller set of values.

slugster
We would always be checking for something being TRUE eventually because only then the 'true' clause will be executed :D That's not what I meant and that's not his criteria. WOW this is getting confusing!
Senthil
A: 

You still have to be careful about things like this:

if ( num % 2 == 1 )
{
  // Number is odd
}

If num is negative and odd then depending on the language or implementation num % 2 could equal -1. On that note, there is nothing wrong with checking for the falseness if it simplifies at least the syntax of the check. Also, using != is more clear to me than just !-ing the whole thing as the ! may blend in with the parenthesis.

To only check the trueness you would have to do:

if ( num % 2 == 1 || num % 2 == -1 )
{
  // Number is odd
}

That is just an example obviously. The point is that if using a negation allows for fewer checks or makes the syntax of the checks clear then that is clearly the way to go (as with the above example). Locking yourself into checking for trueness does not suddenly make your conditional more readable.

Evan Huddleston
@Evan: The odd number thing was just the first example that came to my mind. I didn't think through all possible cases. But I hope readers understand what I am trying to say in the question.
Senthil
I understand. I'm just saying that in cases like this checking for the false case involves 1 check while checking for the true may involve 2 checkes. Checking for the false case seem preferable.
Evan Huddleston
+3  A: 

The problem occurs when, later in the project, more conditions are added - one of the projects I'm currently working on has steadily collected conditions over time (and then some of those conditions were moved into struts tags, then some to JSTL...) - one negative isn't hard to read, but 5+ is a nightmare, especially when someone decides to reorganize and negate the whole thing. Maybe on a new project, you'll write:

if (authorityLvl!=Admin){
 doA();
}else{
 doB();
}

Check back in a month, and it's become this:

if (!(authorityLvl!=Admin && authorityLvl!=Manager)){
 doB();
}else{
 doA();
}

Still pretty simple, but it takes another second.
Now give it another 5 to 10 years to rot.
(x%2!=0) certainly isn't a problem, but perhaps the best way to avoid the above scenario is to teach students not to use negative conditions as a general rule, in the hopes that they'll use some judgement before they do - because just saying that it could become a maintenance problem probably won't be enough motivation.

As an addendum, a better way to write the code would be:

userHasAuthority = (authorityLvl==Admin);
if (userHasAuthority){
 doB();
else{
 doA();
}

Now future coders are more likely to just add "|| authorityLvl==Manager", userHasAuthority is easier to move into a method, and even if the conditional is reorganized, it will only have one negative. Moreover, no one will add a security hole to the application by making a mistake while applying De Morgan's Law.

T.R.
like a lightning bolt from clear sky - explains it very clearly - especially the second piece of code. Best reason I've read. (+1 when I get enough rep :D)
Senthil