<< Profiling with dotTrace | Home | Find out the right directory >>

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:

image

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.

Print | posted on Thursday, March 27, 2008 8:10 PM

Feedback

Gravatar

# re: ReSharper is smarter than me 3/27/2008 8:45 PM Peter Ritchie

Nice conversion, but it lied, it's not using the conditional operator.

Gravatar

# re: ReSharper is smarter than me 3/27/2008 8:54 PM Tim B

GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest==null || presenter.GetServerUrlFromRequest.Value;

Optimized! :)

P.S. your comment form is rejecting valid Plus Addresses, fyi

Gravatar

# re: ReSharper is smarter than me 3/27/2008 8:57 PM Max

Forgive me, but isn't that the same thing as:

GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest ?? true;

Or is my C# rustier than I thought?

Gravatar

# re: ReSharper is smarter than me 3/27/2008 9:00 PM Ayende Rahien

Max,
You see, you are smarter than me and Resharper both!

Gravatar

# re: ReSharper is smarter than me 3/27/2008 9:08 PM alberto

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

Gravatar

# re: ReSharper is smarter than me 3/27/2008 9:13 PM alberto

Ayende beated me to it.

Why would presenter.GetServerUrlFromRequest return presenter.GetServerUrlFromRequest.Value when it's not null?

Gravatar

# re: ReSharper is smarter than me 3/27/2008 9:36 PM Peter Ritchie

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).

Gravatar

# re: ReSharper is smarter than me 3/27/2008 9:45 PM Chris Hynes

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.

Gravatar

# re: ReSharper is smarter than me 3/27/2008 9:48 PM Derik Whittaker

What is also cool is when it tells you you can convert to Lambda's :)

Gravatar

# re: ReSharper is smarter than me 3/27/2008 10:00 PM alberto

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. :)

Gravatar

# re: ReSharper is smarter than me 3/27/2008 10:07 PM Bruno


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.

Gravatar

# re: ReSharper is smarter than me 3/27/2008 10:18 PM Bruno


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.

Gravatar

# re: ReSharper is smarter than me 3/27/2008 10:23 PM Tim B

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.

Gravatar

# re: ReSharper is smarter than me 3/28/2008 5:14 AM Mr_Simple

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.

Gravatar

# re: ReSharper is smarter than me 3/28/2008 10:53 AM Damien Guard

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

Gravatar

# re: ReSharper is smarter than me 3/28/2008 12:11 PM Frans Bouma

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).

Gravatar

# re: ReSharper is smarter than me 3/28/2008 2:24 PM Benny Thomas

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


Gravatar

# re: ReSharper is smarter than me 3/28/2008 2:34 PM Ayende Rahien

Because GetServerUrlFromRequest has a valid meaning of nullable.

Gravatar

# Yet another alternative 3/28/2008 5:27 PM Julian Birch

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 ??...

Gravatar

# re: ReSharper is smarter than me 3/28/2008 6:59 PM Michael Teper

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.

Gravatar

# re: ReSharper is smarter than me 3/28/2008 10:20 PM Jeremy Gray

@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.

Gravatar

# re: ReSharper is smarter than me 3/31/2008 10:49 AM Petar Petrov

The most optimized version for this piece of code is this :
GetServerUrlFromRequest.Checked = true;
if (presenter.GetServerUrlFromRequest != null)
{
GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest.Value;
}
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.

Comments have been closed on this topic.