I am looking at the MvcContrib Grid component and I'm fascinated, yet at the same time repulsed, by a syntactic trick used in the Grid syntax :
.Attributes(style => "width:100%")
The syntax above sets the style attribute of the generated HTML to width:100% . Now if you pay attention, 'style' is nowhere specified, is deduced from the name of the parameter in the expression! I had to dig into this and found where the 'magic' happens:
Hash(params Func<object, TValue>[] hash)
{
foreach (var func in hash)
{
Add(func.Method.GetParameters()[0].Name, func(null));
}
}
So indeed, the code is using the formal, compile time, name of parameters to create the dictionary of attribute name-value pairs. The resulted syntax construct is very expressive indeed, but at the same time very dangerous. The general use of lambda expressions allows for replacement of the names used without side effect. I see an example in a book that says collection.ForEach(book => Fire.Burn(book)) I know I can write in my code collection.ForEach(log => Fire.Burn(log)) and it means the same thing . But with the MvcContrib Grid syntax here all of the sudden I find code that actively looks and makes decissions based on the names I choose for my variables!
So is this common practice with the C# 3.5/4.0 community and the lambda expressions lovers? Or is a rogue one trick maverick I shouldn't worry about?
Source: Tips4all, CCNA FINAL EXAM
This has poor interop. For example, consider this C# - F# example
ReplyDeleteC#:
public class Class1
{
public static void Foo(Func<object, string> f)
{
Console.WriteLine(f.Method.GetParameters()[0].Name);
}
}
F#:
Class1.Foo(fun yadda -> "hello")
Result:
"arg" is printed (not "yadda").
As a result, library designers should either avoid these kinds of 'abuses', or else at least provide a 'standard' overload (e.g. that takes the string name as an extra parameter) if they want to have good interop across .Net languages.
I find that odd not so much because of the name, but because the lambda is unnecessary; it could use an anonymous-type and be more flexible:
ReplyDelete.Attributes(new { style = "width:100%", @class="foo", blip=123 });
This is a pattern used in much of ASP.NET MVC (for example), and has other uses (a caveat, note also Ayende's thoughts if the name is a magic value rather than caller-specific)
Just wanted to throw in my opinion (I'm the author of the MvcContrib grid component).
ReplyDeleteThis is definitely language abuse - no doubt about it. However, I wouldn't really consider it counter intuitive - when you look at a call to Attributes(style => "width:100%", @class => "foo")
I think it's pretty obvious what's going on (It's certainly no worse than the anonymous type approach). From an intellisense perspective, I agree it is pretty opaque.
For those interested, some background info on its use in MvcContrib...
I added this to the grid as a personal preference - I do not like the use of anonymous types as dictionaries (having a parameter that takes "object" is just as opaque as one that takes params Func[]) and the Dictionary collection initializer is rather verbose (I am also not a fan of verbose fluent interfaces, eg having to chain together multiple calls to an Attribute("style", "display:none").Attribute("class", "foo") etc)
If C# had a less verbose syntax for dictionary literals, then I wouldn't have bothered including this syntax in the grid component :)
I also want to point out that the use of this in MvcContrib is completely optional - these are extension methods that wrap overloads that take an IDictionary instead. I think it's important that if you provide a method like this you should also support a more 'normal' approach, eg for interop with other languages.
Also, someone mentioned the 'reflection overhead' and I just wanted to point out that there really isn't much of an overhead with this approach - there is no runtime reflection or expression compilation involved (see http://blog.bittercoder.com/PermaLink,guid,206e64d1-29ae-4362-874b-83f5b103727f.aspx).
I would prefer
ReplyDeleteAttributes.Add(string name, string value);
It's much more explicit and standard and nothing is being gained by using lamdas.
Welcome To Rails Land :)
ReplyDeleteThere is really nothing wrong with it as long as you know what's going on. (It's when this kind of thing isn't documented well that there is a problem).
The entirety of the Rails framework is built on the idea of convention over configuration. Naming things a certain way keys you into a convention they're using and you get a whole lot of functionality for free. Following the naming convention gets you where you're going faster. The whole thing works brilliantly.
Another place where I've seen a trick like this is in method call assertions in Moq. You pass in a lambda, but the lambda is never executed. They just use the expression to make sure that the method call happened and throw an exception if not.
I'm in the "syntax brilliance" camp, if they document it clearly, and it looks this freaking cool, there's almost no problem with it imo!
ReplyDeleteThis is horrible on more than one level. And no, this is nothing like Ruby its an abuse of C# and .Net.
ReplyDeleteThere have been many suggestions of how to do this in a more straight forward way: tuples, anonymous types, a fluent interface and so on.
What makes it so bad is that its just way to fancy for its own good:
What happens when you need to call this from VB?
.Attributes(Function(style) "width:100%")
Its completely counter intuitive, intellisense will provide little help figuring out how to pass stuff in.
Its unnecessarily inefficient.
Nobody will have any clue how to maintain it.
What is the type of the argument going in to attributes, is it Func<object,string> ? How is that intention revealing. What is your intellisense documentation going to say, "Please disregard all values of object"
I think you are completely justified having those feelings of revulsion.
Both of them. It's abusage of lambda expressions AND syntax brilliance.
ReplyDeleteI hardly ever came across this kind of usage. I think it's "inappropriate" :)
ReplyDeleteThis is not a common way of use, it is inconsistent with the general conventions. This kind of syntax has pros and cons of course:
Cons
The code is not intuitive (usual conventions are different)
It tends to be fragile (rename of parameter will break the functionality).
It's a little more difficult to test (faking the API will require usage of reflection in tests).
If expression is used intensively it'll be slower due to the need to analyze the parameter and not just the value (reflection cost)
Pros
It's more readable after the developer adjusted to this syntax.
Bottom line - in public API design I would have chosen more explicit way.
No, it's certainly not common practice. It's counter-intuitive, there is no way of just looking at the code to figure out what it does. You have to know how it's used to understand how it's used.
ReplyDeleteInstead of supplying attributes using an array of delegates, chaining methods would be clearer and perform better:
.Attribute("style", "width:100%;").Attribute("class", "test")
Although this is a bit more to type, it's clear and intuitive.
This is one of the benefits of expression trees - one can examine the code itself for extra information. That is how .Where(e => e.Name == "Jamie") can be converted into the equivalent SQL Where clause. This is a clever use of expression trees, though I would hope that it does not go any further than this. Anything more complex is likely to be more difficult than the code it hopes to replace, so I suspect it will be self limiting.
ReplyDeleteAll this ranting about "horridness" is a bunch of long-time c# guys overreacting (and I'm a long-time C# programmer and still a very big fan of the language). There's nothing horrible about this syntax. It is merely an attempt to make the syntax look more like what you're trying to express. The less "noise" there is in the syntax for something, the easier the programmer can understand it. Decreasing the noise in one line of code only helps a little, but let that build up across more and more code, and it turns out to be a substantial benefit.
ReplyDeleteThis is an attempt by the author to strive for the same benefits that DSL's give you -- when the code just "looks like" what you're trying to say, you've reached a magical place. You can debate whether this is good for interop, or whether it is enough nicer than anonymous methods to justify some of the "complexity" cost. Fair enough ... so in your project you should make the right choice of whether to use this kind of syntax. But still ... this is a clever attempt by a programmer to do what, at the end of the day, we're all trying to do (whether we realize it or not). And what we're all trying to do, is this: "Tell the computer what we want it to do in language that is as close as possible to how we think about what want it to do."
Getting closer to expressing our instructions to computers in the same manner that we think internally is the key to making software more maintainable and more accurate.
It is an interesting approach. If you constrained the right hand side of the expression to be constants only then you could implementing using
ReplyDeleteExpression<Func<object, string>>
Which I think is what you really want instead of the delegate (your using the lambda to get names of both sides)
See naive implementation below:
public static IDictionary<string, string> Hash(params Expression<Func<object, string>>[] hash) {
Dictionary<string, string> values = new Dictionary<string,string>();
foreach (var func in hash) {
values[func.Parameters[0].Name] = ((ConstantExpression)func.Body).Value.ToString();
}
return values;
}
This might even address the cross language interop concern that was mentioned earlier in the thread.
What's wrong with the following:
ReplyDeletehtml.Attributes["style"] = "width:100%";
The code is very clever, but it potentially causes more problems that it solves.
ReplyDeleteAs you've pointed out, there's now an obscure dependency between the parameter name (style) and an HTML attribute. No compile time checking is done. If the parameter name is mistyped, the page probably won't have a runtime error message, but a much harder to find logic bug (no error, but incorrect behavior).
A better solution would be to have a data member that can be checked at compile time. So instead of this:
.Attributes(style => "width:100%");
code with a Style property could be checked by the compiler:
.Attributes.Style = "width:100%";
or even:
.Attributes.Style.Width.Percent = 100;
That's more work for the authors of the code, but this approach takes advantage of C#'s strong type checking ability, which helps prevent bugs from getting into code in the first place.
IMHO, it is a cool way of doing it. We all love the fact that naming a class Controller will make it a controller in MVC right? So there are cases where the naming does matter.
ReplyDeleteAlso the intention is very clear here. It is very easy to understand that .Attribute( book => "something") will result in book="something" and .Attribute( log => "something") will result in log="something"
I guess it should not be a problem if you treat it like a convention. I am of the opinion that whatever makes you write less code and makes the intention obvious is a good thing.
In my opinion it is abuse of the lambdas.
ReplyDeleteAs to syntax brilliance i find style=>"width:100%" plain confusing. Particularily because of the => instead of =
indeed its seems like Ruby =), at least for me the use of a static resource for a later dynamic "lookup" doesn't fit for api design considerations, hope this clever trick is optional in that api.
ReplyDeleteWe could inherit from IDictionary (or not) and provide an indexer that behaves like a php array when you dont need to add a key to set a value. It will be a valid use of .net semantics not just c#, and still need documentation.
hope this helps
Can I use this to coin a phrase?
ReplyDeletemagic lambda (n): a lambda function used solely for the purpose of replacing a magic string.
If the method (func) names are well chosen, then this is a brilliant way to avoid maintenance headaches (ie: add a new func, but forgot to add it to the function-parameter mapping list). Of course, you need to document it heavily and you'd better be auto-generating the documentation for the parameters from the documentation for the functions in that class...
ReplyDeleteI think this is no better than "magic strings". I'm not much of a fan of the anonymous types either for this. It needs a better & strongly typed approach.
ReplyDelete