|
Unsignedness would not excuse the author (value == 0), floats may have (NaN would behave differently)
|
|
|
|
|
Pete O'Hanlon wrote: if (!(value > 0)){ throw new ArgumentOutOfRangeException("....");}
Could we not do this?
if (value <= 0)
There could be a perfectly reasonable explanation: The code maybe like the following previously
<br />
if (!isValid(value)){ throw new ArgumentOutOfRangeException("....");}<br />
Then someone decided to simplify it by replacing isValid(value) with (value>0).
|
|
|
|
|
Sadly not. I know the person who wrote this - he's not that big on any form of refactoring.
|
|
|
|
|
It might just be a case of lazy programming. I've seen code where if statements were initially written in the opposite state of what was intended and then instead of fixing the whole statement, the ! was simply thrown in front.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
That is the most likely explanation.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
I think this is the case also. Every once in a while (once in a blue moon), I do it too. Not that I'm lazy...I just had a logical bug, decided to see if the opposite would work, it did work, so I move to the next problem thinking I'll come back and fix it later.
|
|
|
|
|
Sounds like it was written by a lawyer.
|
|
|
|
|
It may be that the person is using a general pattern of testing whether required input conditions are not met by throwing on their inverse. When preconditions are more complicated, it can sometimes be cleaner say something like:
if (!(foo || bar))
throw new InvalidArgumentException("Neither foo nor bar was valid");
Than to say:
if (!foo && !bar)
throw new InvalidArgumentException("Neither foo nor bar was valid");
If one habitually writes pre-checks based upon preconditions, code will probably read better if one consistently uses the same style. If there will be any need to test preconditions using throw-if-not-met logic, it may be best to do so consistently.
Also, btw, if one is going to be pasting code into something like a web-based forum, it may be helpful to write conditions so as to avoid the less-than sign. My usual rewrite of a less-than-zero condition for web posting would typically be "0 > whatever" rather than "!(whatever >= 0)", but some people may prefer the latter style.
|
|
|
|
|
Of course you could use your form:
if (value <= 0)
But if values greater than zero are OK, and anything else should give an exception, then why not write it in the original form:
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}
This could be considered a more direct translation of the requirement, and therefore more understandable.
Of course, it may just have been a lack of thought, but I'd argue there are times when it would be beneficial not to simplify expressions, particularly when you have a compiler that will reduce it to the same form for you so there is no performance penalty.
|
|
|
|
|
I can top it off! 
static void Main(string[] args)
{
var b = true;
if (!!!!!!!!b != false)
Console.WriteLine("hu?");
else
Console.WriteLine("ha!");
}
A train station is where the train stops. A bus station is where the bus stops. On my desk, I have a work station....
_________________________________________________________
My programs never have bugs, they just develop random features.
|
|
|
|
|
"hu?" is printed. 
|
|
|
|
|
Indeed!
A train station is where the train stops. A bus station is where the bus stops. On my desk, I have a work station....
_________________________________________________________
My programs never have bugs, they just develop random features.
|
|
|
|
|
I use less ! on my messenger
-
Bits and Bytes Rules!
10(jk)
|
|
|
|
|
I wouldn't really call that a "gem".
|
|
|
|
|
Another alternative that I didn't see mentioned in the replies. Was this code hand written or produced by a generator?
I've been playing with the CodeDOM and there are some structures that you can define that would likely generate exactly that code.
|
|
|
|
|
|
It doesn't make much sense for an int, but it might make sense for an int? (Nullable<Int32>).
The ordering operators (<, <=, >, >=) on Nullable<T> will always return false if either operand is null, so !(value > 0) would be equivalent to value == null || value <= 0.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
It's an int, nothing but the int, so help me glod.
|
|
|
|
|
In Visual C++ MFC, there's a CString. And to check if a CString has values, it's "if (!str.IsEmpty()).
So the QA person, who had a degree from Stevens Institute of Technology, sat there and argued with me for 2 HOURS, and brought it all the way up to the VP, because it wasn't the "obvious way to do the logical operation." Didn't matter at all that is was the way Microsoft implemented it, and we had to use it.
|
|
|
|
|
Hopefully you'll be gratified to learn I just used this as an example to one of my students, introducing him to refactoring poor code and [specifically] bad if statements.
|
|
|
|
|
If it stops anybody else making the same mistake, then I'm glad to help.
|
|
|
|
|
Ahhh, teaching him a lifetime skill huh? I hope most of us, who read bad code go:
Is it working correctly?
if no, write a bug and hope someone assigns you ownership so you can fix it.
Hope it's approved, and whoever is assigned the bug takes the time to fix it.
else: Is this egregiously bad, causing performance issues or something else that might cause severe system problems?
if yes, write a bug etc.
else: Do you work in a best practice shop and this violates a best p?
if yes, write a bug etc.
else: Do you work in a best practice shop
if yes, suggest a new best practice reffering to your source and hope it is adopted and the source is addressed.
else: ask yourself if it is worth suggesting to your manager one more thing that could be improved.
if yes, wow, you have a manager who listens to you and acts on your suggestions, or you are working for a new manager, or you are a perrenial optimist.
else: Join the regular ranks, shake your head about the code you see, and go on with your life.
Maybe I have a gift for seeing bad logic.
It took me three days to convince someone this was a bug:
if (current_thread_count <= maximum_thread_count + thread_count_to_add)
add_new_threads(thread_count_to_add);
It took asking for the specification document because I was told this met specifications. I agreed that this exactly met specifications, the only problem is, that specifications aren't asking for what is intended. Huh, what? It took going to the lap, finding out what values they were configured for (maximum_thread_count=120, thread_count_to_add=25) and showing what that would do. (Say your current count is 119, if you add 25 more, the current count would go to 144. You don't want that to happen, right? "Right." Well, 119 is less than 145... "How did you come up with 145" Well, the first value is 120, the second value is 25, added together they are 145. Well, the light dawns, but we continue the exercise proving that the count would go up to 169 because the loop is immediate when if the statement was true and the request is relatively immediate. In theory the count could go to 170.
Anyway, if there is a heavy enough load the service machines should blow up with a thread allocation error. The lab guy who gave us the numbers, pipes up "Oh, yea, we have that problem all the time!"
I'm thinking it would have been nice to know that the problem existed instead of just reading C# code to get up to speed on what the group was doing while waiting for my next assignment. Also, how can the specs get through code review. The person creating the code doesn't see it. I have so much trouble convincing someone there is a problem. I'm passively reading it and it's so bad it almost hurts my eyes.
I heard the problem was significantly reduced, but still occurs. I was on an assignment so it can wait. (Just like everyone else in the group while I was there.)
|
|
|
|
|
Your alternate solution is just as valid. To me the original is perfectly readable, but I'd prefer:
if (value > 0) {//do something useful}
else (throw ...);
Since you have CDO, this is probably too much reading to do before you get to the else statement.
|
|
|
|
|
It is accurate, but inelegant!
------------------------------------
I will never again mention that I was the poster of the One Millionth Lounge Post, nor that it was complete drivel. Dalek Dave
CCC League Table Link
CCC Link[ ^]
|
|
|
|
|
Firstly, with most languages both examples will produce the same code, so there is no need to prefer one from a performance standpoint. So the choice comes down to clarity and depending on the specific situation (and perhaps the individual) either one could be preferable. The first example has the advantage that it more directly reflects the comment. I don't really regard this as a coding horror.
Steve
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
