Don't like the API, go ahead fix it, or not?
Over in the alt.net mailing list, Charlie Poole has said that he thinks that the Playback() method for Rhino Mocks would be better named Monitor().
Without thinking about it, I sent this reply:
public class CharliePreferences { public static IDisposable Monitor(this MockRepository mocks) { return mocks.Playback(); } }
After I sent it, I started thinking about the implications. Both for legacy code and for preferences. I am not sure whatever this is a good approach is a good way to go. Thoughts?
Comments
I think this is a very contrived example, since you're pretty much simply renaming a method. In this specific example, that doesn't really gain you anything at all. Here's why:
Once you understand what Playback() does, renaming it doesn't really bring any significant benefit. The problem with a method like Playback() in rhino mocks is that it forces a metaphor at a place that is totally non-intuitive for someone not already familiar with the tool (at least that was my personal experience when I used it the first time).
Hence, "fixing it" like this when you're already aware of what it does makes no sense. "Fixing it" at the real API level so that the API is more approachable to new users is a much better option, imho (though this can be debatable).
On the other side, I'm sure that extending an API using extension methods is a valid option for more interesting things.
Perhaps fixing up an API that broke historical compatibility by putting old methods that were removed back in?
Hmmm.... I could see something like that in certain circumstances.
I don't think this would really help matters much.
The original developer will already know enough about rhino mocks to be familiar with the terminology, therefore adding no real benefit.
A developer new to rhino mocks wouldn't understand either term without help, and to get help he's likely to 'go to the definition' of Monitor and find it just passes through to Playback(), so back at square one, with an extra layer of un-needed abstraction.
I think it at best adds no real benefit, and at worse makes it worse :)
I agree with Alex. This is harmful, not helpful, assuming that the develop who did this works on a team. Now the developers working the framework are learning less about the mocking tool and more about a specific usage of the tool. Worst of all some of these developers may not even realize that Monitor() doesn't exist on the MockRepository (doubtful I know since you can see it via intellisense).
I just think it's setting people up for failure long term when they move on to their new projects.
Practically speaking, unless he wants to include his special preferences class with all of his test harnesses, this is an exercise in writing soon-to-be-broken code.
Where would this preferences class live?
I'll echo the sentiments of those above; this rather contrived example doesn't make a great case for this approach.
I'll give my +1 to the "he meant fixing the API's metaphor, not the API" argument.
It took me some time to understand the record-playback metaphor and the difference between that and the other ReplayAll()-VerifyAll() usage.
I got both eventually by seeing other people use the framework, but I definitely think there's room for improvement there.
I'd go with "Expect()", "Verify()" and "Complete()", or something else that conveys the fact that you start verifying your expectations as soon as you hit ReplayAll(), and that VerifyAll() only means "if any expectation is still unfulfilled, shout now".
Unless you are going to do this to provide some extra (custom) functionality, I think it causes more problems than it fixes.
I could see something where you wanted to do some stuff before and after you called the Playback method where you would use this technique, but in this simple example, I would stay clear.
Comment preview