When you call a query method that returns true or false, I
highly suggest that you make true results trip conditional logic /
guard clauses / etc. I was driving out some code today in an MVC controller that had a dependency on an type who’s responsibility it is to check for unique email addresses in a database of user which looks like so:
public interface IUniqueEmailValidator{ bool EmailAddressIsUnique(string email);}
All well and good, but you’ll notice that the method returns true in the happy path. That is, if an email address is unique (working on an authentication system), the object returns true.
I use an auto-mocking container when doing TDD with .NET. Most implementations give you back a so-called dynamic mock which just absorbs calls and returns default values on methods that return value types. So in the case above, a dynamic mock will always return false unless you tell it otherwise. I’d end up having to put noisy stub code in all of my other specs. For example:
Dependency() .Stub(u => u.EmailAddressIsUnique(null)) .IgnoreArguments() .Return(true);
In the controller we’re working on right now, there are about 10 specifications that failed because of this default behavior. This is a big problem because the introduction of a new specification results in me having to change a bunch of other contexts which is stinky, fragile design!
An opportunity to make things better! I changed the interface to this:
public interface IUniqueEmailValidator{ bool EmailAddressIsUnique(string email); bool EmailAddressIsTaken(string email);}
The new EmailAddressIsTaken method simply inverts the result of EmailAddressIsUnique. Really frighteningly simple stuff, but, by calling the new method, my test code stays isolated and the special cases don’t break my numerous other specs.
I agree that EmailAddressIsTaken is a lot clearer given how you have to use it. With that in mind, why have 2 methods? That’s the part I’m missing. Why not just have one method that works in the way you want?
I know you said you don’t like “!”, but what else is the EmailAddressIsUnique method going to do but negate EmailAddressIsTaken? That has to happen somewhere if you need the negative result. You’re simply moving that mechanism down a level away from the consumer, but now you’ve locked yourself into a pattern where you have to write 2 methods for every 1 conceptual boolean operation.
In my opinion, it is cleaner to provide 1 method that does exactly what you intend, and let consumers determine if it needs to be negated. I say this not because I don’t want to write extra methods, but because you are solving a problem that has already been solved: negating boolean values.
Write the method as it best pertains to your domain. My guess is that you are only actually checking the return value of EmailAddressIsTaken in your domain code, though you have both methods. YAGNI.
Not to mention that EmailAddressIsTaken does not belong on an interface named IUniqueEmailValidator which has a method EmailAddressIsUnique. #1, uniqueness in general does not have anything to do with ownership. #2, the interface implies EmailAddressIsUnique, but you have to force EmailAddressIsTaken in there. I would rename the interface and just have EmailAddressIsTaken.
#2 is a good point.
A lot of time when you start to write the code you ask yourself ‘what do I need to do?’ and in this case the answer was ‘check if an e-mail is unique’. So the test was written: EmailAddressIsUnique. Makes perfect sense.
But if your production code which you wrote the test for ends up putting a ! right in front of your newly created/tested method, I would change it too (auto mock framework or not!).
…or maybe it was the other way around from the beginning
seems to me you’re switching to state the negative, doesn’t seem like a good idea, and definitely not the right reasons
http://ebersys.blogspot.com/2008/03/do-you-not-want-to-exit-yes-no-cancel.html
@Michael – Excellent points, but I’m still sticking to my guns. A couple of reasons:
1. Do you consider spec/test code to be a first class citizen in your codebase? How coupled is it to your mocking framework or approach? I’ll bet a lot unless you’ve written lots of adapter code and that’s another episode.
2. Read both, left-to-right, and tell me which reads more like English:
if (validator.EmailAddressIsTaken(email)) ExceptionalThing();
if (!validator.EmailAddressIsUnique(email)) ExceptionalThing();
3. I should have written this, but when refactoring I like to introduce duplication that I’ll take out later. Kent Beck calls this technique a “parallel”. ALT+F7 on the old method shows me it’s used in other specs to setup, you guessed it, tons and tons of Stubs to get around exceptional condition.
This is why I’ll try to keep the condition of returning true *when* client code is likely to want to act.
I’m afraid I agree with the others – you’ve muddied up the code to make life simpler (or work around a flaw) for a particular mocking tool. At best, upon seeing this I’m going to think I need to now test if( u.EmailAddressIsUnique(email) && !u.EmailAddressIsTaken(email)) each time I use an implementation. If I go digging, and see the code is redundant, I’ll flag it for refactoring. If you’re not a fan of ! you can use u.EmailAddressIsUnique(email) == false.
I’ll also disagree that a mocking tool or testing framework is a first class citizen to the platform. Either should be replaceable without requiring changes to the “real code that does stuff”.
@Rainer – The other effect is that we don’t use to use a “not” everytime we ask the method. The second method is also more intention revealing and language oriented. The purpose of my little service there is to determine if the email address is unique, therefore it should answer true. It’s nice that this aligns with the mocking framework (which I consider to be a first class citizen in my platform).
Not really, to be honest. You change the code in a way that it matches the default behavior of your mocks. I can’t help it but it has a bad taste for me. Shouldn’t it be the other way round? If the mocks stink, I would get rid of them and use something else.
@liam – Nope, you’re not missing. It’s really just a very simple change. I tend to think using ! is an anti-pattern.
I fixed the cut-n-paste error, thanks. See the comment to @ayende above.
Well, there was a typo, the top code sample should have read:
public interface IUniqueEmailValidator
{
bool EmailAddressIsUnique(string email);
}
A dynamic mock will return false when this is called w/o a stub. A bunch of specs exercise a controller that calls this dependency and would also need a stub setup. By asking if the address is taken (inverting the true/false of the method) the other specs don’t change.
Make sense now?
you’re returning false and changing your interface to support working more easilily with your mocking framework. anything more than that — am i missing something? the interface examples are the exact same by the way.
You make me feel stupid, I don’t get what you are doing here.