Fluent Refactoring
I have just made the following refactoring, starting from here:
Owner owner = GetAccountOwner(); Lookup to = new Lookup(EntityName.queue, Settings.Default.ControlQueueID); Lookup from = new Lookup(EntityName.systemuser, owner.Value); Guid regarding = PostState.new_parentpolicy.Value; Picklist priority = PickLists.EmailPriority.Type.High; SendMail(from, to, title, body, owner, regarding, priority);
I moved here:
Owner owner = GetAccountOwner(); Email .Owner(GetAccountOwner()) .From(EntityName.systemuser, owner.Value) .To(EntityName.queue, Settings.Default.ControlQueueID) .Regarding(PostState.new_parentpolicy.Value) .Priority(PickLists.EmailPriority.Type.High) .Title(title) .Body(body) .Send();
Not much in terms of lines of code, but my sense of aesthetics is pleased, at least. And at least it ensures that we won't have this:
SendMail( new Lookup(EntityName.systemuser, GetAccountOwner().Value), new Lookup(EntityName.queue, Settings.Default.ControlQueueID), title, body, GetAccountOwner(), PostState.new_parentpolicy.Value, PickLists.EmailPriority.Type.High);
Comments
I like it. We've been doing some similar refactorings on our current project. It's certainly more readable.
I like it especially when you run into API's like:
Send(string from, string to, string message)
replaced by
SendMessage("blah").From("someone").To("else")
The aesthetics are pleasing. In terms of enforcing proper usage of the api though, doesn't it reduce the ability of the compiler to help you? I suppose in the original, it would be easy to mess up by getting the 'to' and 'from' arguments the wrong way round, so that's not perfect because the compiler couldn't help you there. But, in the refactored example, can the fluent interface be engineered to ensure the user passes all necessary arguments? Or, does if have to provide defaults... for example if the .Body(body) line was inadvertantly missed out could the compiler be made to generate an error? And, if in the future you changed the number of arguments required by SendMail and recompiled, could the compiler point out all the call sites that also needed modifying when using a fluent interface like this?
I'm not saying I don't like it! It looks nice, but I'm just wondering if there are trade-offs being made in making things look nice.
I didn't really think of Scott's point, but the more I think about it, Ayende's approach gets close to that of the dynamic language world. That being said, the way to validate that the proper values have been set is simple: a unit test.
Ayende, what do you think of the alternative method of taking a simple struct as the input for parameters? With C# 3.0's support for initializing fields & properties with constructors (new A() { B = 1, C = 2 }), this would be relatively simple. This is how a lot of Javascript libraries work and allows for optional parameters quite easily. Moreover, you can make a [Required] attribute for these structs and use reflection to verify that they are populated properly. If you are hardcore and want performance++, you can probably verify at an IL level that the correct fields/properties are set.
Having used a lot of Javascript lately (namely Mootools), Luke's approach would make a lot of sense in this case. Both look much better than the original approach at least...
Scott Langham,
There are several ways to handle that.
One is to handle it by stepping through it.
So Email would have only Owner() method, which would return an object with only To() method, which would return an object with only From() method, etc.
That is a hassle, and I don't like it.
The simplest way is to throw an exception if you don't have everything in place.
Luke,
Yes, named parameters would probably do a lot of good here.
MonoRail makes extensive use of them, and I certainly like it.
Don’t you think this is one of those areas where the C# and VS fall short and you could be refactoring yourself into a corner.
Both the first and third examples are fine, I agree on the readability issues and this is such a common problem that the fine folks at MS should be looking at VS / C# solution. If VS would align the declarations into columns the first would be readable. We could also have initialization inferred from the constructors which the compile could catch.
I agree with the comments about testing but I think going too far in the rely on the tests more than the compiler direction is the wrong path, there should be a balance. Use the compiler to catch as many problems as possible and more tests will pass the first time.
I'm not sure I like it. This is after all a relatively straightforward statement to write and I find the original code is perfectly readable.
Moreover, when the from/to/subject/etc... values require multiple lines to compute because they aren't simple expressions we're back to the original code again using variables. But now we're also saddled with a noisy "fluent" interface that isn't pulling its weight in that context.
Perhaps you'd like writing code in Self or SmallTalk syntax. (That'd be a fun project: IronSelf on the CLR!)
emailService.SendEmailTo: to
From: from
Regarding: policy
...
Hehe. On the other hand... maybe not. :-)
Personally I dislike the "fluid interfaces" movement because it conflicts directly with the semantics of immutable value objects. For example, I'm used to reading code like this:
date = DateTime.Now.AddDays(1).AddHours(2).AddMinutes(3)
and understanding that each succesive call is returning a new object.
This is in direct conflict with the fluid interface model in which each call inflicts a mutation on the object and returns the same object. I agree it leads to pretty code, but I think you pay the cost in terms of conflicting mental models.
Jeff,
Well, since my idea would be:
email.Send( To: "foo", From: "bar" )
I don't have an issue with that at all
jr76,
It simply requires a different mode of thinking.
I assume you don't have an issue with:
stringBuilder.Append("abc").Append("def")
I like the API but not so much the method chaining as it can make debugging a real pain.
Owner owner = GetAccountOwner();
Email
2 calls to GetAccountOwner?
Why do those fluent interfaces always remind me of
With email
.Owner = "Bla"
'etc
End With
-Markus
Why is this better than using a parameter object?
Using methods as if they were properties seems weird to me, at least name them SetTitle() etc instead of just Title()?
/Mats
I really dig fluent interfaces, but find some people look at them and say "so?".
I blogged about it here:
http://weblogs.asp.net/bsimser/archive/2007/07/26/being-descriptive-in-code-when-building-objects.aspx
But generally the response I got was "use properties" or it's just "syntactic sugar". Well yes it might be, but it's a helluva lot easier to read, especially in a test when I really want to fly through reading the codebase.
Personally I prefer to use a fluent interface via a Builder or something when writing code. It's just much easier to understand and doesn't take that much more effort. I favor readiblity over more code in this case.
I like fluent interfaces but there is a big cost and in many cases I think you'd need to provide both interfaces (or an Expression Builder).
In this case I think Mats has a point, why not create a parameter object called MailMessage and create it so the required fields are passed to the constructor and the rest can be set with properties. That tells you what is required and lets you provide the other values if you want to.
Anyway I think fluent interface is like everything else, it seems great then when you try it out you realize that without good language/tool support its a lot of extra effort. Not necessarily a bad thing and I do think it could be useful in a some cases.
David Laribee has a good example of this style of interface for sentence "builder": http://ayende.com/Blog/archive/2007/09/18/Fluent-Refactoring.aspx
@Bil - Why can't people add comments to your older posts, I've tried to add comments to entries on your blog but it only seems to let me comment on the really new ones.
Of course, you could be using Title() as a verb. But then I shudder at the linguistic horror that follows when we also consider Body() to be a verb.
/Mats
More seriously, aren't fluent interfaces more relevant when you need to encapsulate a flow?
Since it doesn't matter if you set the title or the body first, a fluent interface seems like a bad fit?
/Mats
@Mats - Interesting point, where have you observed fluent interfaces being used most successfully?
Elegant. I love fluent interfaces since they guide you to the next practical set of options based on the context.
Comment preview