Ayende @ Rahien

It's a girl

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.

Comments

Peter Ritchie
03/27/2008 06:45 PM by
Peter Ritchie

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

Tim B
03/27/2008 06:54 PM by
Tim B

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

Optimized! :)

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

Max
03/27/2008 06:57 PM by
Max

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

GetServerUrlFromRequest.Checked = presenter.GetServerUrlFromRequest ?? true;

Or is my C# rustier than I thought?

Ayende Rahien
03/27/2008 07:00 PM by
Ayende Rahien

Max,

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

alberto
03/27/2008 07:08 PM by
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

alberto
03/27/2008 07:13 PM by
alberto

Ayende beated me to it.

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

Peter Ritchie
03/27/2008 07:36 PM by
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).

Chris Hynes
03/27/2008 07:45 PM by
Chris Hynes

Peter -- from reading the code, I'd assume that GetServerUrlFromRequest is a Nullable, in which case the ?? is the perfect solution to this. If not, tim b or alberto's solutions are the other alternative.

Derik Whittaker
03/27/2008 07:48 PM by
Derik Whittaker

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

alberto
03/27/2008 08:00 PM by
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. :)

Bruno
03/27/2008 08:07 PM by
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.

Bruno
03/27/2008 08:18 PM by
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.

Tim B
03/27/2008 08:23 PM by
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.

Mr_Simple
03/28/2008 02:14 AM by
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.

Damien Guard
03/28/2008 07:53 AM by
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

Frans Bouma
03/28/2008 09:11 AM by
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).

Benny Thomas
03/28/2008 11:24 AM by
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

Ayende Rahien
03/28/2008 11:34 AM by
Ayende Rahien

Because GetServerUrlFromRequest has a valid meaning of nullable.

Julian Birch
03/28/2008 02:27 PM by
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 ??...

Michael Teper
03/28/2008 03:59 PM by
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.

Jeremy Gray
03/28/2008 07:20 PM by
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.

Petar Petrov
03/31/2008 07:49 AM by
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.