Posts Tagged ‘aop’

If it’s for test then it has to be good

Wednesday, March 25, 2009

You are dealing with some old code: it’s not too bad, you have definitely seen much worse. There’s a class within a home-grown framework that doesn’t allow you to test some application code. You spend some time understanding how to change things to make it more testable. After a while you come up with the idea of adding a method to that class solely for testing purposes – it’s not a fantastic thing, but the alternatives are frightening and this will make a lot of code much more testable. Because you are a good person you want to make it very clear that that code is intended to be used for testing only, not in real life:

public void setMickeyMouseForTest(Class cheeseClass,
                                  String cheeseName, Mouse mouse) {
    cheeseClass = resolveAssociatedClass(cheeseClass);

    if (getLocalFactory().getCheeseName(cheeseClass) != null)
        throw new RuntimeException("Use standard setMickeyMouse instead");

    mouses.put(cheeseName, mouse);
}

This method is similar to the real thing:

public void setMickeyMouse(Class cheeseClass, Mouse mouse);

Method setMickeyMouse determines cheeseName by calling getLocalFactory().getCheeseName(cheeseClass), while method setMickeyMouseForTest assumes that cheeseName is not set (even stronger: if it is not null it throws an exception) => instead, it uses the parameter provided in the call.
In the end, the difference between the two lies in the way a certain condition is handled in the code: the method written for testing purposes is capable of dealing with an exceptional condition differently. You’ve created that method exactly to simplify the amount of setup required for tests, so that it doesn’t matter if getLocalFactory().getCheeseName(cheeseClass) is not capable of providing a cheeseName.

So, the code is committed, lots of tests are now possible, the team celebrates your genius.

Some weeks later you’re pair programming with a colleague. You’re still dealing with the same framework, but this time it’s a different corner-case. After lots of mumbling, back and forth, debugging sessions, you superficially look at the signature of that method, obviously without remembering what you have done a month before. You just vaguely remember that it was capable of dealing in a better way with a corner-case. And the method is called “…ForTest” so it has to be good, in the end we are testing indeed.

It doesn’t work. An exception is thrown. The exception hints to “Use standard setMickeyMouse instead…“. Oh gosh, let’s take a look: ops, I know what it is, we’ve forgotten to set cheeseName for cheeseClass in the local factory. Let’s do that.

<type> <type> <type> <type> <type> <type> <type> <type> <type> <type> <type> <type> <type> <type> <type> <type>

Damn, it’s still not working. What is it happening?!?

<debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug> <debug>

It’s very strange.

getLocalFactory().getCheeseName(cheeseClass) is not null, so how can it possibly say that it is null instead?!?

<more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug> <more debug>

Oh my gosh! The exception is actually thrown WHEN getLocalFactory().getCheeseName(cheeseClass) is not null, not viceversa!

What happened is that you and your colleague just wasted 20′ because you misread “!= null” (reading “== null” instead). What does that tell you?

It told me a few things:

  • Managing null values is always evil and error prone. At the very least, checking for a null disrupts the business logic and it makes the code less readable, ultimately more likely to break.
  • It’s too easy to misread a check based on != vs ==. Not only that: the fact that an exception is being thrown if the expression evaluates to true hints that the check is to verify if the value is null, because that is what you would typically do when you are adding a guard to your code. Which brings me to the next point.
  • Moreover, the intent of the code is not clear. It’s not enough to call a method “somethingMeaningfulForTest“, that is the suffix “ForTest” is misleading if the method is not truly applicable for every test. This is even reinforced by the fact that the exception message is not enough communicative.
  • As usual, test code has to follow the same standard of quality as application code, which means readability, expressiveness, simplicity, lack of duplication. This is particularly important for accessory code that is written to support tests.

So, in the end, how should that method be called? What about:

public void setMickeyMouseForTestWhenThereIsNoAvailableCheeseName(
    Class cheeseClass, String cheeseName, Mouse mouse);

The underlying precondition is that method getLocalFactory().getCheeseName(cheeseClass) returns null for this code to execute properly. This should be made clearer with a checked exception being thrown by the method, or, even better, by a custom annotation that marks the method with the defined precondition. In Java land this doesn’t go very much beyond documentation purposes for a variety of reasons:

  • First of all there are limitations on the attributes that an Annotation can exhibit. Specifically, only String, Class and primitive types are allowed, not even object version of scalar type like Integer can be used.
  • Second and foremost, annotations can be queried to provide metadata about code, but they don’t really affect code execution. An annotation is useful only to the degree it is recognized by the runner executing some code. There is in the end no native and effective support for Design By Contract in Java and there will probably never be. A useful result can be achieved by combining together AOP with Annotations, but I have mixed feelings about it => in the end I always have the impression that the mumbo jumbo typical of the supporting infrastructure it’s there just to circumvent a limitation that should be inherently addressed by the language itself.

Or shouldn’t it?


Follow

Get every new post delivered to your Inbox.

Join 258 other followers