Omer van Kloeten's .NET Zen

Test on a valid x, do not implement tests based on not this and not that etc., always implement tests which state what you want, not what you don't want.

So if you want a valid X, you should test for a valid X. A valid X is a value > 8, option 3 is thus prefered.

I don't think that using two negations self-documents the code any better than just writing the code without the negation. With either of the three, you'd have to add a comment line above explaining why you have to have (myVariable > 8), so I don't think the options for what code you use matters in the above case--none of the above choices are self-documenting.

I think a clearer option would be to use a function call. Though it doesn't seem useful to add a function to replace a single boolean operation, try:

if (!isValidX(myVariable)) //etc

bool isValidX(int x) {
return (x <= 8);
}

Then you put comments by the 'isValidX' function to explain why this rule exists.



But with all this said, all of the above options work, and given enough time, anyone can figure out what any of those options mean, so it's all marginal benefits.


Pete

Self-documentation is not an end in itself, it is a means to an end. What is the end? Clarity. While the developer may not be able to prevent convoluted requirements statements from being documented, (s)he can and ought to distill goofy expressions into consice logic. Extrapolate this example into a different, but related discipline: building architecture and engineering. It is possible that the customer could say to the architect I want a building that should not not have any curves. The architect's responsibility is to restate the syntax of the requirement to facilitate the construction while still capturing the intent of the customers wishes: I want a building with some curves.

Everyone,

The problem may be in the requirements, I'll grant you that, but this is a requirement that has appeared in documents.
I agree with you that this can never be considered self documenting code and must have an explanation. On the other hand, writing code that is as close to the requirements as possible is best, as long as it doesn't hurt readability (it adds to it most of the time) or performance.

Frans,

It wasn't stated that the requirement was a valid X. It was stated that the requirement was _not_ a valid X, which has invoked this discussion. I apologize if I didn't make myself clear enough.

Pete,

Taking X as a black box, we could use a function and negate that. However, it was decided that checking for X is not going to be a function.

Christian,

The example you give is good, but lacking, since it does not contain the fact that X is a black box definition. When taking "not X", it is not analogous to "not not having curves", since the two "not"s are easily understandable as double negation to anyone. When considering "not X", on the other hand, one must first look into the definition of X to understand it completely.

I think I'd be loathe to maintain the kind of code you write.

The test for validity is that "myVariable" is not greater than 8.

I would code choose option 3 all the time. All your ideas about double negation might be great in your mind (because you're the one who came up with it), but anyone who has to maintain your code will doubtfully fully understand your "intention" at first.

I think this is a big reason to write code that is self documenting *and* is easy to read, if nothing more than the fact that ~80% of total IT budgets is targeted at maintaining existing applications.

While option 1 states the requirements exactly, it is unreadable and that disqualifies it automatically.
Option 2 is in mid state between stating the requirements and readable code, hence it lacks in both and thus discouraged. We want to achieve a state where we commit to readable code, and that's not it still. It also does not totally document the requirements, which means we just lost the one reason we "let" it be half unreadable.
option 3 is best because the code is more readable, and stands up to the requirements.
However, there would still need to be a comment on it, no matter which option you choose.

Let's take up the issue of maintainability that 'Anonymous' started:

What if this check wasn't just that simple check, but a big ass messed up check and then someone decided that they _do_ want to put this check into a function? Would you like to start looking into how the person who wrote the code 'optimized' it or would you like to just mark the whole damn thing and write the function's name instead?
In my opinion, using option 3 is a serious plus.
If you're talking readability, I agree that some people might think that this is less readable (that's why I wrote this post asking for opinions), but a comment would be made anyway so this would not matter eventually...

I may disagree with you guys (apparently with all of you, so far), but everyone's entitled to their own opinion. :)

p.s. I don't understand why you, or anyone else for that matter, might identify yourselves as 'Anonymous'. I don't attack people personally, since this is a civilized discussion. It's your right, but it looks like you don't want me to know who you are and that kinda sucks.

Its not HOW you meet the requirements, its that it does. While option 1 is most "to the letter" of the requirements, it is by far the most unreadable.

All 3 may compile to the same IL and thus run equally fast on a silicon processor, but the meatbag compiler in my skull took at least tenfold longer to parse 1 and 2 compared to 3.

Highschool Algebra taught me that you always simplify the expression, this includes refactoring out the negatives. Therefore you would always use "x > 8" or "x <= 8" depending on whether your testing for validity or invalidity




PPS: Even though this is beside the point of the excercise, the correct answer is encapsulate the the logic in its own method, cuz tomorrow the client is gonna change their mind and instead of value <= 8 it will be where value exists in database table Z.

Hey guys, I would like to poll your opinions about having two functions isValid(x) and isNotValid(x) which equals to !isValid(x) but is 'more' readable. I know, crazy and stupid, I just want to hear from you guys.

Thanks

When training new developers I always try to pound several basic rules into their brains, one of them being: "Always assume that the person who will be tasked with maintaining your code is a psychopathic killer who knows where you live. "

Ask yourself, would that code cause him to pay you a visit? I would say that option #1 is a definite yes, option #2 is a maybe.

BTW, another of my rules is "In a large project, things never appear *twice* in the code - they will either appear only once, or hundreds of times." Your value of 8 sounds like a good candidate for something that will need to be search-replaced in a million places later on.

I like the idea of factoring out the validity test into is own function. I that that's by far the most readable, and probably won't impact performance since the compiler will most likely inline the function call anyway.

I think RebelGeekz suggestion, while well-meaning, is overkill. The whole purpose of a general-purpose negation operator is to specifically remove the need to implement two versions of every function. IsNotValid() is needless syntactic sugar.

It's not that I don't not agree with number 1 - it's just that by not agreeing I might not not agree with those who don't agree with those who do agree that by not agreeing I am agreeing with those who do.

Omer,
I respectfully disagree with your assesment. Mathematically, all we have in both instances is an expression which can be factored out.
A double negative is a primordial grammatical sin. Coding a solution to requirements is not about copying syntax, it is about copying semantics. In this case, you are advocating translating the syntax of the requirementas definition verbatim into your programming language of choice. This is a bad idea because it does nothing to improve code readability, option 3 is just as correct as option 1, and you are propagating confusion. Furthermore, self-documenting code, clear code, or auto-generated documentation can never be a substitute for a requirements document.

Will the real 'Anonymous' please stand up!

Seriously, the reason I chose to post is I'd rather make friends than enemies, and for something like this I'd rather unload both barrels at once to stop something like this from ever happening in the first place.

I have to go with #3. If you insist on matching the spec precisely, you're asking your spec writers to be good coders. In my own programming I frequently find that the spec is unnecessarily convoluted, and I much prefer to find a way to code it cleanly.

Addy, if your other basic rules are expressed as vividly as your psychopathic killer rule, I'd like to hear them :)

To Anonymous: The problem with anonymous criticism is that it automatically gets rejected out of hand - if someone doesn't even stand behind what he says, why should anyone listen? It's always easy to criticize when you don't have to back up your claims, just sniping from the bushes. Just not the best recipe to be taken seriously.

I would go back to the user and ask "is it OK to just say "greater than 8"?

<style>
<!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:"Antique Olive";
panose-1:0 0 0 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman";}
@page Section1
{size:595.3pt 841.9pt;
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.Section1
{page:Section1;}
/* List Definitions */
ol
{margin-bottom:0cm;}
ul
{margin-bottom:0cm;}
-->
</style>

<p class=MsoNormal>Well, I'll jump in with the majority.  Obviously (3) is
preferable out of the choices... but:</p>

<p class=MsoNormal>&nbsp;</p>

<ol style='margin-top:0cm' start=1 type=1>
<li class=MsoNormal>deciding on validity should be a function in it's own
right, because </li>
</ol>

<ol style='margin-top:0cm' start=1 type=1>
<ol style='margin-top:0cm' start=1 type=a>
<li class=MsoNormal>it's more readable</li>
<li class=MsoNormal>it will be used in multiple places, and </li>
<li class=MsoNormal>the rules will be changed</li>
</ol>
<li class=MsoNormal>I wouldn't put a constant like &quot;8&quot; anywhere in
code! remember, programmers only know 3 numbers - 0, 1 and many  - and
they are the only ones that can be hardwired! &lt;/p&gt;</li>
</ol>

<p class=MsoNormal>&nbsp;</p>

<p class=MsoNormal>so... we are left with what PDS said above – a nice IsValidX
function, which is completely readable, completely maintainable.</p>

It's funny how people keep telling me that I should write this as a method, even though the question was not about it and I specifically wrote in the post:
"It was decided that there should not be a method returning a boolean value whether or not an integer was a valid X or not"
But that's beside the point. What I wanted to say was that we don't live in a perfect world and these rules were the ground rules for this problem's solution and could not be avoided.

It's interesting to hear everyone's input. It seems everyone thinks that option 3 is best always. I'll have to take this into considiration the next time I come face to face with this kind of situation, even though I am still not so sure whether or not it really is the best way to go...

Why wouldn't you just save everyone the trouble of trying to understand your code and simply write:

(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(!(myVariable + 1 - 1 > (4+4)*2/(2)))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))


That way the IL can stop being such a lazy-ass and do some serious parsing!

I apologize for not reading the entire thread, and this may have been stated before but the following:

"It was decided that there should not be a method returning a boolean value whether or not an integer was a valid X or not and 8 should not be a constant."

is just asking for trouble.

Just as an analogy, not to make light of your plight:

"It was decided to shoot our developers in the foot. It was also decided to enter them into a dancing contest."

To get into a discussion of which type of dance would be more popular with the judges seems like missing the forest for the trees falling on your head.

Once again, I'm just trying to add some humour to a serious matter and in no way intend to offend.

Udi,

Some times you can get some stupid requirements. These were a couple of them.

This was a actually a true problem in one of the code reviews I was asked to attend. People there were very sarcastic and refused to listen to half of what I said in the false pretext that it was just a waste of time and unnecessary.
The question I raised here is also one of the things I encountered there.

Perfectionism and systems design @ Fog Creek Software Forums