21 Answers
21

Amongst my rules of thumb for writing clear code is: put all side effects in statements; non-statement expressions should have no side effects.

Your first version of the program clearly follows this rule. The second version clearly violates it.

An additional thought: if I were to read code like the code you've displayed, I would naturally assume that the purpose of the code was to build up a lazily-evaluated structure which represented those operations -- this is exactly why query comprehensions in C# 3 are built in this way. The result of the query expression is an object representing the deferred application of the query.

If your intention is to capture the notion of "execute these side effects in a deferred manner at a later moment of my choosing", then this is a sensible approach. Essentially what you're building up is a side-effecting monad. If your intention is merely to provide a different syntax for the eagerly executed code, then this is just confusing, verbose and unnecessary.

Another guidelines for cases like this IMHO, is that: every design should solve a problem. It's unclear what problem the fluent API here actually is trying to solve.
–
LBushkinDec 16 '09 at 19:20

3

In what statements? The statement expressions inside the lambdas? When I read that I read "this method is capturing a side-effecting operation that can then be saved and re-applied any number of times at any time of the method's choosing." That's completely different than the statement, which you know executes once, not zero, one, two or a thousand times. And you know it executes when control reaches that point in the program, not at some other time of some other method's choosing.
–
Eric LippertDec 16 '09 at 20:54

10

I'm of the opinion that List.ForEach() was not a great idea. How is "list.ForEach(foo=>{bar(foo);}" appreciably more valuable than "foreach(var foo in list){bar(foo);}" -- it's the same characters in a different order! There's no compelling representational power added by making ForEach an expression.
–
Eric LippertDec 16 '09 at 22:10

I see no advantage to this besides being confusing to the reader. With respect to my fellow answerer, I would like to know on what planet this is more readable. As far as I can tell, the first version has more or less perfect readability, whereas this is fairly readable, but makes the reader wonder whether there's some strange magic happening within With and WithIf.

Compared to the first version, it's longer, harder to type, less obvious, and less performant.

The "perfectly obvious" solution is the original one. Everything else is unnecessary, and amounts to little more than a coding trick put in place just to prove what you can do. This kind of thing is incredibly asinine, no matter what name you call it. A rose by any other name...or in this case...a turd by any other name.
–
GusDec 16 '09 at 17:56

I guess I fail to see what the new versions get you. The original is pretty clear and is less wordy. I would guess that it would be faster as well. I would avoid using (abusing?) language features like this unless there is a clear benefit.

One more vote for "not useful". The With extension method doesn't do anything except wrap up sequenced statements with a method. C# already already has a built-in function for sequencing statements, its called ;.

Similarly, the WithIf wraps an if-statement without any modification to the control flow. From my point of view, you're only inviting yourself to methods like:

They're just different coding styles, what do you mean by "too big a departure"? Departure from what? From what you're used to? Only you can decide that. I will say that VB's With block has done more harm than good to code readability, and I would not try to replicate the behavior in C#, but that's just my preference.

I pretty much always use this for FindControl (yeah, strongly typed to RepeaterItem, it doesn't have to be, but that's the only thing I ever use it for anyway):

I guess a departure from more common C# coding styles. Certainly a departure from my organization's coding standards.
–
andleerDec 16 '09 at 17:41

While I'd still say that this is mostly up to your preference, I'll add that if your organization has coding standards, you ideally shouldn't depart from them at all =)
–
David HedlundDec 16 '09 at 17:45

I am more comfortable with the first version. It takes less time to read and understand. I agree that the extension methods are also fine if you are familiar with it and also familiar with the With method, but what’s the benefit of it in this case?

"This will save you time in debugging sooner or later." Only if your editor doesn't have auto-indenting. Otherwise, I prefer no braces for single-line expressions. Looks cleaner to me.
–
bsneezeDec 16 '09 at 19:09

I'd vote for single-line expressions on the same line as the if statement
–
ScoregraphicDec 22 '09 at 9:00

I predict the whole "fluent interface" fad will be the "hungarian notation" of the 2000's. I personally think it doesn't look very clean and it runs the risk of becoming very inconsistent if you have multiple developers each with their own preference.

My 2 cents: It looks fine, my only comment is that "With" kind of implies something like "Where" or "Having" when you are actually setting a property. I'd suggest a method name of something like "Do", "Execute" or "Set" but maybe thats just my odd world view.

With() could be used to extract or "get" values as well. .With(d => fred = d.Text)
–
andleerDec 16 '09 at 17:40

I just mean that "With" (to me) makes it look like you're checking for a value eg: .With(d=>d.Text=="Foo") as opposed to executing an action eg .With(d=>DoStuff(d)) could be: .Execute(d=>DoStuff(d)) or .Do(d=>DoStuff(d))
–
grenadeDec 16 '09 at 17:48

I say stick with the first version without the extension methods or lamba expressions. These are relatively new concepts so not many developers will have a handle on them yet outside their use in data retrieval/manipulation from a database. If you use them you may have a hit on maintenance cost. It is nice to say "read up if this is Greek to you"; but in real-life that may be the best approach.

This isn't a reason to hold back inovation. It is a reason to help teach your peers and push inovation forward.
–
Matthew WhitedDec 16 '09 at 17:42

4

It is not practical or wise to refrain from the right thing because other people might not have an appropriate working knowledge of the language. The question at hand should be whether this is the right thing or not.
–
mquanderDec 16 '09 at 17:45

I tend to agree with these comments. Learning new tricks and concepts is to programming as moving forward and breathing is to a shark. You have to do it or you die.
–
andleerDec 16 '09 at 18:11

Regarding a "Fluent Interface" C# already has a great syntax for initializers which is (IMHO) better that trying to use the fluent style. Of course, in your example you are not initializing an new object, you are changing an existing one. My whole expertise with Fluent interfaces comes from a 30 second scan of wikipedia, but I think that JeeBee's answer is more in the spirit of Fluent programming, though I might change things slightly:

In certain circumstances thoughtfully constructed fluent interfaces can be very useful. First, because the developer is presented with a limited number of options they are (typically) easy to use correctly and difficult to use incorrectly. Second, because of the sentence like structure they can be a nice clean way to declare your intentions, especially when building complex objects.

I have found fluent interfaces to be very useful when developing test code in which it is often necessary to build lots of domain objects with slight variations. I have also used them successfully as a way to introduce the decorator pattern and to eliminate excessive method overloading.

If anyone is interested in learning more about fluent interfaces, I suggest checking out this work in progress by Martin Fowler.

So you wouldn't like this for (var i = -1; ((++i < array.Length) ? ((T?)(array[i] = set)) : null) != null; ) ; ... (that's a joke by the way... I wouldn't want to debug it either... but it does work)
–
Matthew WhitedFeb 4 '10 at 18:34