ReSharper is smarter than me
Given the following block of code:
if (presenter.GetServerUrlFromRequest!=null) GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest.Value; else GetServerUrlFromRequest.Checked = true;
Resharper gave me the following advice:
And turned the code to this piece:
GetServerUrlFromRequest.Checked = !(presenter.GetServerUrlFromRequest!=null) ||
presenter.GetServerUrlFromRequest.Value;
And while it has the same semantics, I actually had to dechiper the code to figure out what it was doing.
I choose to keep the old version.
Comments
Nice conversion, but it lied, it's not using the conditional operator.
GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest==null || presenter.GetServerUrlFromRequest.Value;
Optimized! :)
P.S. your comment form is rejecting valid Plus Addresses, fyi
Forgive me, but isn't that the same thing as:
GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest ?? true;
Or is my C# rustier than I thought?
Max,
You see, you are smarter than me and Resharper both!
No,I think it's not.
It would be equivalent to:
GetServerUrlFromRequest.Checked = (presenter.GetServerUrlFromRequest == null) ? true : presenter.GetServerUrlFromRequest.Value;
But I don't like that syntax. Maybe Resharper didn't like it either and changed its mind. That would be smart. xD
Ayende beated me to it.
Why would presenter.GetServerUrlFromRequest return presenter.GetServerUrlFromRequest.Value when it's not null?
No, GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest ?? true; doesn't work, that doesn't set the value of Checked to Value at all.
You can't do what you need with the null coalescing operator because you're dealing with two variables (GetServerUrlFromRequest and GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest ?? true;), not one (GetServerUrlFromRequest).
Peter -- from reading the code, I'd assume that GetServerUrlFromRequest is a Nullable<bool>, in which case the ?? is the perfect solution to this. If not, tim b or alberto's solutions are the other alternative.
What is also cool is when it tells you you can convert to Lambda's :)
Oh, you are right, the similarities in the names fooled me, it probably is a nullable bool and then, Max solution would be the smartest way. :)
I believe the better way is to do it INLINE, instead of 4 lines of code -- and they are 4 lines using C# embbeded statements, imagine it using all the curly braces.
It´s a simple decision, in my opinion its easier to read this way:
*** variable = [make decision here using ternary or coalescing operator]
I don´t like to scroll my screen to read small, very simple portions of logic...
Of course you should to evaluate the cpu cicles too (not like an OS kernel developer), but in this case I guess they are the same.
Also sometimes it´s cool to stop typing and stay looking at the screen for some moments (may be minutes), thinking about what you are designing/developing at that time.
Compact code with judicious use of embedded statements and C# powerfull operators help me to do that.
I was originally going to post the ?? solution but mine included the .Value from the original code, and that would have thrown a null ref exception. Good thinking Max.
Mr_Simple says YOU are smarter than ReSharper because you kept your original syntax.
If you have to stare at code to understand it, the code it too complicated.
Besides, it's more fun to write new code than stare at old code.
The Resharper code looks simpler to me because the intent is clear after just scanning the bit before the =.
In the original code you have to read through all three lines to realise it is only setting .checked.
[)amien
One tiny remark: it's in general better to have a boolean expression which tests for equality and not NOT equality in the if statement. So instead of:
if(someVar != value)
{
// A
}
else
{
// B
}
you should do:
if(someVar == value)
{
// B
}
else
{
// A
}
The reason is that if you have to alter the code to add a new clause to the if, you run the risk of creating a bug because adding the clause with 'AND' to the first if is actually an OR for the second if, while your intentions probably were adding it with AND to the second (and thus needing an OR for the first).
I like to bitch about things and I know this is besides the topic.
But why do u not use the logic in the presenter instead? Don't let GetServerUrlFromRequest be an Nullable.
Then you can do simpel things like:
GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest;
You get nicer code and let the presenter do the logic handling.
My 2 cents
Because GetServerUrlFromRequest has a valid meaning of nullable.
var gs = presenter.GetServerUrlFromRequest;
GetServerUrlFromRequest.Checked = gs == null || gsValue;
Of course, if the type of gs is bool?, gs ?? true works, owing to a rather entertaining bit of the spec of ??...
RS 4 includes a number of suggestions that I think make code less readable most of the time. I've turned them down a notch and am much happier for it.
@Michael - Agreed. I generally find overly dense code to be a code smell and anti-pattern, much to the chagrin of some dense-code developers that I've worked with.
My logic goes a bit like this: making ALL code unnecessarily dense (in terms of whitespace collapse as well as things like overly-complex ternary usage) tends to hide code that is dense for reasons that could indicate errors and/or a general need of clean-up / refactoring.
I want that kind of dense code to stand out (especially during quick scanning) from better code, but anything beyond the simplest cleanest of ternary operator usages, null coalescing usages, etc. tends to decrease the signal to noise ratio and slows scanning in general. Not to mention that a lot of developers find the more advanced operators hard to understand, even when reading slowly, let alone scanning.
The most optimized version for this piece of code is this :
because of the instruction pipeline we have 50% chance to keep fetched instructions. It's true that in the original version we have one condition check and one assignment but in this version we have one condition check, on assignment and 50% probability to to have one more assignment. Even with this additional assignment we have better performance.
Comment preview