It's not the most obvious piece of logic.
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
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:
/* Either foo or bar must be true */
if (!(foo || bar))
throw new InvalidArgumentException("Neither foo nor bar was valid");Than to say:
/* Either foo or bar must be true */
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.
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
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.
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
I can top it off! :laugh:
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.
-
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.
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.
-
I can top it off! :laugh:
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. :)
-
"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.
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
I wouldn't really call that a "gem".
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
It doesn't make much sense for an
int, but it might make sense for anint?(Nullable<Int32>). The ordering operators (<, <=, >, >=) onNullable<T>will always returnfalseif either operand isnull, so!(value > 0)would be equivalent tovalue == null || value <= 0.
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
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.
-
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.
Hand written, sadly.
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
-
It doesn't make much sense for an
int, but it might make sense for anint?(Nullable<Int32>). The ordering operators (<, <=, >, >=) onNullable<T>will always returnfalseif either operand isnull, so!(value > 0)would be equivalent tovalue == 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.
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
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.
-
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.
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
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. :)
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
-
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.
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
-
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.
Sounds like it was written by a lawyer.
-
So, today I came across this gem:
// If the value is NOT greater than zero, throw an exception.
if (!(value > 0))
{
throw new ArgumentOutOfRangeException("....");
}Could we not do this?
if (value <= 0)
:rolleyes:
I have CDO, it's OCD with the letters in the right order; just as they ruddy well should be
Forgive your enemies - it messes with their heads
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
-
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.
