Monday, February 09, 2009

To Assert or Not To Assert

by Miško Hevery

Some of the strongest objections I get from people is on my stance on what I call "defensive programming". You know all those asserts you sprinkle your code with. I have a special hate relationship against null checking. But let me explain.

At first, people wrote code, and spend a lot of time debugging. Than someone came up with the idea of asserting that some set of things should never happen. Now there are two kinds of assertions, the ones where you assert that an object will never get into on inconsistent state and the ones where you assert that objects never gets passed a incorrect value. The most common of which is the null check.

Than some time later people started doing automated unit-testing, and a weird thing happened, those assertions are actually in the way of good unit testing, especially the null check on the arguments. Let me demonstrate with on example.
class House {
  Door door;
  Window window;
  Roof roof;
  Kitchen kitchen;
  LivingRoom livingRoom;
  BedRoom bedRoom;

  House(Door door, Window window,
            Roof roof, Kitchen kitchen,
            LivingRoom livingRoom,
            BedRoom bedRoom){
    this.door = Assert.notNull(door);
    this.window = Assert.notNull(window);
    this.roof = Assert.notNull(roof);
    this.kitchen = Assert.notNull(kitchen);
    this.livingRoom = Assert.notNull(livingRoom);
    this.bedRoom = Assert.notNull(bedRoom);
  }

  void secure() {
    door.lock();
    window.close();
  }
}

Now let's say that i want to test the secure() method. The secure method needs door and window. Therefore my ideal would look like this.
testSecureHouse() {
  Door door = new Door();
  Window window = new Window();
  House house = new House(door, window,
             null, null, null, null);

  house.secure();

  assertTrue(door.isLocked());
  assertTrue(window.isClosed());
}

Since the secure() method only needs to operate on door, and window, those are the only objects which I should have to create. For the rest of them I should be able to pass in null. null is a great way to tell the reader, "these are not the objects you are looking for". Compare the readability with this:
testSecureHouse() {
  Door door = new Door();
  Window window = new Window();
  House house = new House(door, window,
    new Roof(),
    new Kitchen(),
    new LivingRoom(),
    new BedRoom());

  house.secure();

  assertTrue(door.isLocked());
  assertTrue(window.isClosed());
}

If the test fails here you are now sure where to look for the problem since so many objects are involved. It is not clear from the test that that many of the collaborators are not needed.

However this test assumes that all of the collaborators have no argument constructors, which is most likely not the case. So if the Kitchen class needs dependencies in its constructor, we can only assume that the same person who put the asserts in the House also placed them in the Kitchen, LivingRoom, and BedRoom constructor as well. This means that we have to create instances of those to pass the null check, so our real test will look like this:
testSecureHouse() {
  Door door = new Door();
  Window window = new Window();
  House house = new House(door, window,
    new Roof(),
    new Kitchen(new Sink(new Pipes()),
           new Refrigerator()),
    new LivingRoom(new Table(), new TV(), new Sofa()),
    new BedRoom(new Bed(), new Closet()));

  house.secure();

  assertTrue(door.isLocked());
  assertTrue(window.isClosed());
}

Your asserts are forcing you to create so many objects which have nothing to do with the test and only confuse the reader and make the tests hard to write. Now I know that a house with a null roof, livingRoom, kitchen and bedRoom is an inconsistent object which would be an error in production, but I can write another test of my HouseFactory class which will assert that it will never happen.

Now there is a difference if the API is meant for my internal consumption or is part of an external API. For external API I will often times write tests to assert that appropriate error conditions are handled, but for the internal APIs my tests are sufficient.

I am not against asserts, I often use them in my code as well, but most of my asserts check the internal state of an object not wether or not I am passing in a null value. Checking for nulls usually goes against testability, and given a choice between well tested code and untested code with asserts, there is no debate for me which one I chose.

27 comments:

  1. Hey,

    I live over in .NET world, but read the blog for more test knowledge.

    I've read a great deal about mocks on this blog - surely the last example would not be as bad if you used mocks instead of instantiating everything?

    Furthermore, over in .NET world there's a new term called an automocker - I guess you could view it as a hybrid between an IoC container and a mocking framework.

    It would let you do exactly as you described your ideal world, but without the null values.

    Check out this link for more info or search the web for automocker...

    http://blog.eleutian.com/CommentView,guid,762249da-e25a-4503-8f20-c6d59b1a69bc.aspx

    :-)

    ReplyDelete
  2. I don't really have an informed opinion one way or another on the use of asserts.

    But, if you really wanted to keep your asserts in the constructor another way to approach the problem would be to use builders to construct your test objects. Something like:

    Door door = new Door();
    Window window = new Window();
    House house = HouseBuilder.new().with(door).with(window);

    house.secure();

    assertTrue(door.isLocked());
    assertTrue(window.isClosed());

    I think that a pretty clear way to tackle the problem. Where the HouseBuilder provides some sensible defaults for the other variables. This also solves the problem of not creating objects in an invalid state

    ReplyDelete
  3. I disagree because in your case one needs to know about the fact that the lock() method you are testing only uses the door and window, and does not use anything else.

    This results in your testers having to know about the implementation, which should not be the case... maybe my house has a folding roof just like convertibles have, and I have to lock that too, but it is null.

    ReplyDelete
  4. Were you going somewhere with the external API? It seems to trail off mid-sentence although I was interested in it.

    ReplyDelete
  5. I totally agree with earlNameless..

    The only reason you know the secure methods only needs those two objects is because you know the code.

    What if Secure() activated motion sensors in the Kitchen, LivingRoom and BedRoom?

    Maybe it is just a bad example, but from what I see, this test code needs to know wayyyy too much about what is going on already..

    I would probably have some sort of ISecurable interface and would test each object independantly.. You then don't give a crap what the house is doing since you know that all it is doing is calling ISecurable.Secure(), and each implementer you have tested, right?

    If the ability to add in an interface is not an option, then it really becomes a moot point, you cant change the code anyway.. So just get an IoC container in there and make the hurt go away :)

    ReplyDelete
  6. Misko, I completely agree with you. Those asserts (other null asserts and guard clauses) are redundant and get in the way (not only in the way of the test but of the reader of the production code). Not to mention in the case in question, it'd be the responsibility of your DI framework to not pass in a null (in production), not the responsibility of this class.

    ReplyDelete
  7. This is exactly why people should use an AMC (Auto mocking container) when testing. Your tests become much less brittle, much smaller (less setup), and more intention revealing.

    Using a builder is another solution but also comes with cost of just pushing the setup code to a common a place that still needs to be maintained. Typically you would only keep those builders in test assemblies which provides little value when you consider an AMC can do all that for free.

    When you add a StairWell parameter to the constructor none of your existing tests will break with the AMC. With the builder approach or non-builder approach it will need to be refactored.

    Removing the asserts all together is the most dangerous as now you are allowing for objects to be created with an inconsistent state. Very shortly you will start seeing those null tests scattered throughout the system and SRP and LOD start to scream when that happens.

    ReplyDelete
  8. In the code I've written and seen, we assert not null before an object is used, not when they are stored. Is this reasonable? That would put Assert.notNull(door) and Assert.notNull(window) in your secure() method. Verifying the presence of your objects before using them makes much more sense to me than verifying the presence of all objects you might be concerned with maybe someday.

    ReplyDelete
  9. This comment has been removed by the author.

    ReplyDelete
  10. To Howie,
    the null-checking assertion are made to ensure that the object's state is right before using it.
    The example above is quite simple, but assume that you have a code where there are several places
    in which the "kitchen" or "bedroom" fields are set independently one to the other.

    If you don't check the consistance of the state in those places, you won't be able (whitout a
    complete suite of unit test) to easily identify "where" your kitchen has been set to null.

    I agree to earlNameless... in that way to test object's methods, you need to know the method's
    source code (behavior) to determine the entities involved in the test. Mysko teached us about the
    importance of the explicit declaration of the dependancies: so, if our House uses, in a part of the
    code, just a small part of the House's dependencies, it's a sympthom that there is something wrong
    with House design: maybe, you don't need an House but just an HouseLockingSystem, which
    needs to know only about House's doors and window (not about kitchen, or bedroom, or everything else...)

    ReplyDelete
  11. Hi,

    Nice discussion.

    My thoughts :
    You need to differentiate between a valid null and an invalid null

    make sure you set up a constructor for a valid house only with a door and a window parameter, fill the rest with "Null objects" http://en.wikipedia.org/wiki/Null_Object_pattern

    than you don't need the clumsy constructor, also you don't need a lot of nulls, you keepm the logic internal to your house and you know the difference between a valid null and invalid null.

    What do you think ?

    ReplyDelete
  12. Sometimes ago I blogged about ways of indicating the absence of an object:

    http://rnaufal.livejournal.com/10017.html

    One interesting comment I received about using Assert instead of null checking:

    "There's a document in the JDK docs titled Programming With Assertions that says (http://java.sun.com/j2se/1.4.2/docs/guide/lang/assert.html#usage):

    Do not use assertions for argument checking in public methods"

    ReplyDelete
  13. Great discussion, Misko.

    There may be another alternative to explore: tag parameters with nullness annotations such as in JSR-305 (http://jcp.org/en/jsr/detail?id=305).

    It would have some distinct advantages:
    1. People using the constructor know that null is not supported just by looking for @Nonnull parameters in the constructor's signature rather than having to look up the javadoc.
    2. You don't have to bother with writing and maintaining statements like "null is not permitted" in the javadoc, which often seem to be lacking or out of synch with the actual code.
    3. All the Assert.notNull statements can be removed from the constructor, making it much cleaner again.
    4. Users with up-to-date static analysis tools (such as FindBugs) get a warning right away when passing null instead of waiting for an assertion error at runtime.
    5. You don't have to write a bunch of tests that check that null really is prohibited as stated in the javadoc. (This is what led me to search for this functionality in the first place - these tests were getting to be really numerous and time-consuming).

    For the test, you could still pass in null where you want to and suppress the generated warnings. You of course run the risk of using the constructor in a manner that is clearly stated as not being supported, and are relying upon knowledge of the internal implementation as others have mentioned. I guess in that case, at least the test code has been warned of its risky usage and can be updated if the internals of the class under test change.

    That said, I'd feel a lot more comfortable if these warnings were generated by the compiler itself instead of relying upon third party tools (which other users may lack, thereby allowing them to miss the warnings).

    I'm also not sure how to balance this against using a DI framework since I don't have much experience with any of them. I think Misko has a good point that it clutters the test though, even if the grunt-work of creating the objects is handled elsewhere.

    So I guess the fundamental question is, do you want to:
    1. Take a bit of a gamble by ignoring some of the contract for the class under test so that the test can be simplified -or-
    2. Follow the contract exactly as stated up-front by providing all the necessary dependencies and making it a bit more difficult to sift through the internals of a failing test (due to the existence of unrelated objects)?

    ReplyDelete
  14. Knowing the implementation in order to write the test is expected. This is TDD not TAD right? If the roof needs locking, change the specification, the test fails, then change the implementation so everything turns green.

    The test is the verification of how the system works. I personally find it easier and quicker to scan the specifications to find out how things work rather than reading implementation code.

    As for removing assertions from the constructor I have to again disagree. When a client comes around trying to consume the api there needs to be a fast fail if the object is in an inconsistent state.

    @Kyle K... I would like to see a full blown DBC specification for java, something like spec# in .net land. http://research.microsoft.com/en-us/projects/specsharp/

    ReplyDelete
  15. I would venture that a solution here would be to pass mocks (auto-generated by some library), which are by definition non null.

    Plus when the code change and your method suddenly needs to do something on another one of its dependencies, you get a message like "unexpected call to mock("kitchen").clean" instead of "NullPointerException" ...

    Cheers
    PH

    ReplyDelete
  16. I do disagree Miško .. The problem is this case is the logic of assertion behind the program as a programmer they have to check all the possible input for argument . I am a tester but i would recommend assertion is to be more in the case of web programming and interactive modules.

    ReplyDelete
  17. Several comments have taken issue with the fact that the test code Misko discusses would have to know details about the implementation. While it is true that there is a certain brittleness where test code knows internal details - this is unit testing, which is, by nature, whitebox, not black box. If you're using mocks (instead of fakes) then you definitely should know about the internals... specifically, you're testing that your code does what you expect, given certain conditions. You do have to know what those conditions may be, and you can exclude conditions that you know won't be.


    If someone comes along and changes the implementation such that those assumptions aren't valid, that will break the tests. But, really, that's the point. You have a suite of unit tests which form a regression. If I change the behaviour, I change the meaning of the test. I therefore have to also change the test to be valid for the new assumptions.


    A lot of this depends on whether you're testing behaviour or input-output, and that's a test-design philosophy. TDD doesn't assert behaviour driven, but use of behavioural mocks does tend to indicate this approach.

    ReplyDelete
  18. I'm very skeptical of this.

    Null is not an object. Null is a way of making obects optional. You are really asking that *all constructor arguments be optional*, which seems wrong to me.

    If the object is documented as doing this, then it must put all kinds of crazy null checks that verbosify the code and can't possibly do anything sensible anyway. I know this is fairly standard in Java, but that doesn't make it a good thing.

    That Java even *lets* you pass in a non-object for all reference types is a flaw in the langauge's type system. ML and C++ for instance have methods for specifying required reference parameters. I hear that future version of Java will getting @NonNull as standard as well?

    Maybe it makes testing a little easier, but making all constructor arguments optional strikes me as a good way to make the implementation of the class more verbose than it needs to be.

    Also, making constructor arguments optional will in some cases make you *return* null maybe even unintentionally, which is *always the wrong thing to do*.

    ReplyDelete
  19. Can't you just switch to the assert keyword and disable assertions when running unit tests?

    ReplyDelete
  20. "Now let's say that i want to test the secure() method. The secure method needs door and window."

    You are testing the contract. If the secure() method needs to go to the LivingRoom and get a torch, that's not your problem. In order to operate correctly, the House needs this stuff. Mock it! If you wrote to interfaces, you'd be able to mock at top level and not worry about "creating so many objects". Then a house with a BrightlyLitDoor (implements Door) would not need a torch from the LivingRoom, and a House with a MagicWindow would not even need the Window to be secure.

    ReplyDelete
  21. In Spring, I usually do this in InitializingBean.afterPropertiesSet(). That way the framework can validate that my bean is wired properly, but I don't necessarily need to call this method if I don't want to in my tests.

    ReplyDelete
  22. I think there are two separate issues here:
    1. do you allow the user to pass NULL values instead of objects.
    2. To assert or not to assert.

    First you need to decide if it is valid for the method to accept NULL values.
    Then you need to assert all inputs for valid values (be it NULL, or what ever other values).

    ReplyDelete
  23. Very interesting discussion.

    I would agree with neronotte, maybe there is a flaw in the design of the house. Maybe the fact that I want to ignore some parameters of the constructor, because I know my test won't need, them is a 'smell'? There could be a hidden object creeping around...Though not always, as it seems reasonable that not every method of an object uses every instance field.

    As with the 'to assert or not' question, Effective Java, Item 38 suggests that methods that are not to be exposed should use Java's assert construct, which can be turned off. Publicly exposed methods should explicitly check and throw exceptions (as in Miško's example). That seems sensible to me. Therefore, I would check that I'm not passed null objects when I explicitely forbid them (i.e. in the javadoc) in my API, and add unit tests for those corner cases. And I'd rather use Dummy Objects instead of null (based on the test double taxonomy at http://xunitpatterns.com/Mocks,%20Fakes,%20Stubs%20and%20Dummies.html), to make the tests work in both situations. (Or maybe we have a point in favour of the setter injection here ;-)

    It's interesting, however, to see how unit testing has an impact on design and coding practices. We're trying to exercise very specific parts and/or behaviours of a software that are meant to exist in the midst of other pieces. Unit testing entails extra thinking so as to be able to isolate each and every part of a software and to use them easily outside of their expected environment.

    ReplyDelete
  24. This comment has been removed by the author.

    ReplyDelete
  25. I'm totally disagree with you, each object must make sure its can operate the designed functionality, can use see if you don't have an eyes?

    Well, what about

    class TestHouse{

    private House house;

    public TestHouse(){
    //create a new house object
    }
    @Test
    public void testSecureHouse() {
    //do you test
    }
    }

    or

    public void testSecureHouse() {
    //Now Assert aware and will not
    //Throw exceptions
    Assert.setTesting(true);
    //do your tests
    }

    ReplyDelete
  26. Hello Misko,

    One sentence in your post really got me there, and i think worth a comment by itself: "null is a great way to tell the reader, "these are not the objects you are looking for"".

    I have to say I disagree. the behaviour of null object is to cause a null pointer exception when accessed, and a possibly cryptic stack dump. there has to be a very good reason to choose this path. why not create a mock object instead of null, using constructor injection, that exports more information and possibly even communicates meaningful results back to the testing code when accessed?

    (for bonus points, a framework could possibly generate these 'tripwire' items for you automagically using reflection, and even restart the test with a real item instead of a tripwire, just in case)

    Compare these two outputs:

    output 1:
    java.lang.NullPointerException at line 44
    (then, 32 more lines down the road...)
    at getRoom() (line 63) (aha! we crashed on the room being null... - too bad this text isn't part of the stackdump!)

    output 2:
    TripWire: getRoomName() got unexpectedly accessed in testSecureHouse() - Consult business logic to see if this makes sense and fix your test or your bug. stackdump logged.
    exception caught - Restarting test with real LivingRoom object.
    test passed.

    and the calling constructor difference?

    case 1:
    House house = new House(door, window,
    null, null, null, null);

    case 2:
    House House(door, window,
    TripWire(roof), TripWire(kitchen), TripWire(livingRoom), TripWire(bedRoom)).

    ReplyDelete

The comments you read and contribute here belong only to the person who posted them. We reserve the right to remove off-topic comments.