Ayende @ Rahien

Refunds available at head office

Making code easier to read

I didn't like this code:

image

I find this much easier to read:

image

Comments

Peter Ritchie
11/13/2008 06:23 PM by
Peter Ritchie

How would you differentate between or conditions and and conditions?

Peter Ritchie
11/13/2008 06:28 PM by
Peter Ritchie

So, the And is implied... If you had to support both AND and OR conditions would you still do it that way?

Eric Hauser
11/13/2008 06:50 PM by
Eric Hauser

The second one may be easier to read, but isn't it iterating over the collection twice?

Ayende Rahien
11/13/2008 06:55 PM by
Ayende Rahien

No, it isn't.

Evaluation it lazy, ater all.

dru
11/13/2008 07:15 PM by
dru

+1 Ayende

Justin Rudd
11/13/2008 07:42 PM by
Justin Rudd

It's cleaner, but there is a subtle issue. You have to remember that Where calls are applied in reverse order (last to first).

In the former code containment in "Items" is checked before the "Type" check. In the latter code, the checks are the same order (Where is applied from last to first), but it reads differently. In this particular case, it may not matter, but in something like this...

var nonNullNameOfLength10 = AllItems.Where(x.Name != null && x.Name.Length == 10);

It reads well...oddly (IMO).

AllItems.Where(x.Name.Length == 10).Where(x.Name != null);

I've seen people stack the Where calls in the same order as the conditions. And that causes the code to break (you could be checking the length of a null name).

Ricky
11/13/2008 08:21 PM by
Ricky

Why not

var irems = AllItems

.Where(x=> x.Type == parent.Type)

.Where(x=> !parent.Items.Contains(x);
Ricky
11/13/2008 08:21 PM by
Ricky

oops forgot last parens :)

var irems = AllItems

.Where(x=> x.Type == parent.Type)

.Where(x=> !parent.Items.Contains(x));
Duarte Nunes
11/13/2008 08:23 PM by
Duarte Nunes

@Justin Rudd

The calls are, indeed, applied in reverse order. But that means Where(x.Name != null)'s MoveNext() will call, before running its own code, Where(x.Name.Length == 10))'s MoveNext(), which could then throw a NullReferenceException. So, the code executes in the correct order (otherwise, as seen in your example, it would be really counterintuitive).

The only difference between Ayende's listings is that the latter, while much more readable, incurs one extra method call.

@Ricky

I'm guessing, readability.

Rik Hemsley
11/13/2008 10:39 PM by
Rik Hemsley

I would probably format this as:

var items = AllItems.Where

(

x=>

  x.Type == parent.Type

  &&

  ! parent.Items.Contains(x)

);

Yes, I use a lot of vertical space, probably too much...

BTW I've said it before, but SubText doesn't seem very good for a developer's blog where code is posted.

Rik Hemsley
11/13/2008 10:40 PM by
Rik Hemsley

Gah it screwed up the formatting... of course.

Benny Thomas
11/13/2008 11:41 PM by
Benny Thomas

I like your thinking, Ayende!

Maybe you should make your own extension called And, that calls where, then it looks even better.

Bunter
11/14/2008 02:46 AM by
Bunter

For me, first version is easier. I like the sound of "and" the && makes in my brain. But I would use in this case

.Where ( x => ( x.Type == parent.Type && !parent.Items.Contains(x)) )

I'm master of unnecessary parentheses.

Ben Hall
11/14/2008 03:01 AM by
Ben Hall

I don't agree, and I really don't think this adds much terms of readability.

Personally, I find the code much more difficult to read due to the fact that we are implying the operator being used . It also means that if we want to use a mixture of && and || in the code, we will have two different approaches or having to flip conditions which sound people could easily misread.

Given the two following examples, I would always bit the latter.

var items = AllItems

.Where(x=> x.Type == parent.Type)

.Where(x=> !parent.Items.Contains(x));

var items = AllItems

.Where(x=> x.Type == parent.Type && !parent.Items.Contains(x));

Peter Morris
11/14/2008 09:29 AM by
Peter Morris

I do something similar when I perform OCL queries

Person.allInstances

->select(givenName = 'Peter')

->select(familyName = 'Morris')

OR conditions need to be in their own select. I find it easier to read because a condition with lots of & needs to be mentally processed as a single element when reading the source, whereas multiple WHERE or ->SELECT can be read as smaller elements and I find them easier to read.

Ray Booysen
11/14/2008 04:28 PM by
Ray Booysen

Based on the code below, you can see the where conditions are not done in reverse order:

Random r = new Random();

List <double values = new List <double();

for (int i =0; i < 10000; i++)

{

values.Add(r.NextDouble());

}

var result = values

.Where(d => { "Inner".Dump(); return true; })

.Where(d => { "Outer".Dump(); return true; });

result.Dump();

Ray Booysen
11/14/2008 04:55 PM by
Ray Booysen

And of course, this was done with LinqPad. ;)

PandaWood
11/15/2008 03:24 AM by
PandaWood

@Ben Hall - I think the point you're missing is that the Ayende's version has less to read, less to think about.

I could understand it and re-read later, just as fast. The alternative you provided, always requires more time parsing in my brain.

I get a bit worried when people don't see this. I do believe that your brain would read it quicker as well.

I am starting to think that the only people who prefer that type of unnecessarily verbose sort of statement, these days, are those whose are allowing aspects of their personality influence what should be a more scientific choice.

Assuming we're all in the business of programming for "business" Ayende's choice has got to be the right one.

If you're programming something lower level, maybe I'll bend a little.

configurator
11/16/2008 05:01 PM by
configurator

What reverse order are you guys talking about? Could you elaborate a bit?

norbi
12/25/2008 11:29 PM by
norbi

Hi*

Apart from readness, which for me second options creates more confusion, from my simple test it seems that these codes are not executing the same.

First is always evaluating the first statement, which is checking collection and that equality with parent.type is first true.

Second on the other hand applies filters as they are (hmm, it would be strange to apply pipeline in reverse order), and this is valid but with query expresions not operators. So if you would like to achive reverse execution you should write like this

var items = from x in AllItems

where x.type == parent.type

where parent.Items.Constains(x) == false

select x;

Regards!

norbi
12/25/2008 11:42 PM by
norbi

Sorry for confusion, it's to late :/, execution is the same of course with expressions/operators.

So still Ayende is it by mistake this order?

regards!

Comments have been closed on this topic.