Ayende @ Rahien

Unnatural acts on source code

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

Tomas Restrepo
12/11/2007 02:26 AM by
Tomas Restrepo

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.

Aaron Erickson
12/11/2007 03:20 AM by
Aaron Erickson

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.

Alex McMahon
12/11/2007 10:44 AM by
Alex McMahon

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 :)

John Chapman
12/11/2007 11:15 AM by
John Chapman

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.

David Mays
12/11/2007 03:05 PM by
David Mays

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.

Avish
12/11/2007 04:01 PM by
Avish

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".

Chris May
12/11/2007 05:20 PM by
Chris May

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.

Comments have been closed on this topic.