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.
31 responses so far ↓
Isn’t the real problem that Assert.notNull is a static method? If you would pass an Assert object (using regular dependency injection) and use an instance method, you could easily use an Assert dummy implementation which ignores certain calls such as notNull.But maybe dependency injection is too much for such a commonly used thing as assertions. Could this be a case were global state is actually justified? E.g. calling Assert.ignoreNotNull before running your tests? I’m not sure.But in any case, the Assert calls introduce a form of global state. I think an Assert object would really make sense: for example, if assertions do fail in a production environment, one could do detailed logging and try to recover, instead of just crashing as in a debug environment.
It’s worth pointing out that if you are manifesting your “House” as a service rather than a value object (or bean, or component – whatever you want to call it), then by using a dependency injection framework and constructor injection you will have the equivalent prevention of nulls, since the constructor-arguments are (in that case) requests for dependencies. A good DI framework will fail to manifest the object if it can’t satisfy the dependencies, so… no nulls. The null-check in the object itself is really a mis-assignment of responsibility. It is the responsibility of the factory/DI Container to manifest a valid object, not of the object itself. I know that’s sort of the point of the article in a way, but I wanted to state it more explicitly… this sort of assertion in constructors means “the constructor is doing too much work” and the fact that there’s more than variable assignment is a big symptom (or smell).
@Ruben
Global state is not justified. Christian makes a good point, that it is not the responsibility of the class to worry about its construction. It is hard to realize that something we have been doing for so long may actually be detrimental.
“It is hard to realize that something we have been doing for so long may actually be detrimental.” mmm maybe that is a way to explain why this is not something I am swallowing easily.
I think your code example may be a little unfair to make your point though, misko. You have been an advocate of ‘Null Objects’ in the past, that being objects which ‘do nothing’ to pass around rather than passing null values, and I think they apply here quite nicely.In the 2nd/3rd examples, what if it looked like this (no idea if comment will garble this code):testSecureHouse() { Door door = new Door(); Window window = new Window(); House house = new House(door, window, new NullRoof(), new NullKitchen(), new NullLivingRoom(), new NullBedRoom()); house.secure(); assertTrue(door.isLocked()); assertTrue(window.isClosed());}It seems to clearly convey that only Door and Window are used for secure. It plays nicely with DesignByContract asserts. Together I think unit tests and DBC fit like a glove and a hand: DBC defines the contract of how an object/method works, and tests test the contract. I find it very useful to make tests that fail the contract (and require this failure to pass) to cover both ends of the bargain.Would like to hear your thoughts on this. I am afraid it appears to me that you are throwing the baby out with the bathwater in the interest of making tests a little easier to write.
The code was mangled, here is a readable version:http://pastebin.com/f37e89dedTwo requests, misko, since I enjoy your blog so much. First setup comments to not strip whitespace or provide a code tag. Secondly change your CSS so it is not so impossible to see what I am highlighting when I try to select some text in your post!
I prefer to keep the asserts in place. If building a LivingRoom is tedious, create a helper class that provides a reasonable test data: House house = new House(door, window, TestObjects.ROOF, TestObjects.KITCHEN, TestObjects.LIVING_ROOM, TestObjects.BEDROOM);
Miško, this is the first post of yours that I strongly disagree with. Essentially you are saying that your class has a set of invariants that must be satisfied for one call chain, and a different set for another chain. You have basically described different concerns/responsibilities and should consider splitting your class. For instance in your contrived example your House has a “securing” responsibility and presumably an “accommodating” responsibility. You can then better generalise the House object (what if you need a House with more than one bathroom or bedroom, or multiple doors and windows) and get smaller, more reusable units that are easier to test. Lastly your code doesn’t use interfaces, making it more difficult to test and use mocks/stubs/null objects in place of the concrete instances you need to create, so, what you describe above is not a unit test but an integration test.
misko sometimes you seem to contradict yourself. Door ,Window, Roof , Kitchen, LivingRoom and BedRoom are all dependencies which the House requires, because if it did not require them why would they be in a constructor. Lets just pretend that they are not required what would the House look like then? Well it would have an overloaded constructor which took different types and if this were the case then maybe I could see your point; yet not in the example shown.
I would also like to ask why in your videos and the blog you seem to be talking about producing code which is easy to test. Why not follow TDD and code the tests first, for example in one of your videos you were even talking about two people doing the coding. One to write the code and one to write code to test the code, surely google does not work like this?
@Liam,
yes TDD is the answer and I TDD all of my code. However getting people to TDD is lot harder than getting people to write testable code.
Misko, I love your blog, but on this issue I have to disagree with you. When using your House class in a ‘real program’, none of those params should be null. So why should the test allow them to be? There’s no reason to have to pass ‘real’ objects there – just set up some mocks to be passed in. That will also have the added benefit of the mocks being able to throw exceptions if any method calls are made on them, which is useful for validating that they indeed are not being used.
I agree global state is never justified. But isn’t that the core of the problem then? Assert has static methods, which introduces global state, and because of that it is not testable. An Assert object would overcome these problems, and have the additional benefit of keeping the assertions at debug time, but not at test time?
I agree with what you say, but not with your example: you shouldn’t be doing defensive programming in a method that doesn’t require something.
The non-null requirement is a class invariant when it is in the constructor. Putting the non-null requirement in methods that actually use objects makes sense, that would be a method precondition.
What I’m saying is that you should allow your house to be built with a null roof, but you shouldn’t allow the call to any method that interacts with the roof if the roof is null.
Hi Misko,I agree with JF Bastien. You do not have to put the assertions in the constructor, but in the concrete method.Great blog!Ivan
I agree with you … however, how do you resolve your advice with the one given by josh bloch in “How to design a good Api and why it matters” talk (http://www.youtube.com/watch?v=aAb7hSCtvGw) where he says that if you pass in say a null value in the constructor, you should error out asap instead of letting the user proceed with an expensive operation and then throwing an exception at the end:So he says:Roof roof = null;House h = new House(…., roof, ….);h.expensiveAndLongOperation();h.operationThatUsesRoof(); // throw NPEis bad, butRoof roof = null;
House h = new House(…., roof, ….); // throw IllegalArgumentExceptionis good.I personally like your approach to testing but I see the point made by Josh as well.
argh .. the pseudocode got messed up
I think it would make a lot more sense to leave the constructir asserts in place, and use mock objects or a test data builder to simplify the tests.
Dear Miško,Although I always agree with your opinions, I’m afraid I cannot be with you in this case. I also think that your example is wrong because (IMHO) if you can call the constructor with nulls or even NullObjects, that smells to those collaborations are optional, so you can simply take them away from the constructor and put setters for them (or use the Builder pattern). In both cases you can keep the asserts for those properties that never can be null. Actually you should move them to the setters (or builder methods).And also I’m not sure what you mean when saying (in one of the comments you made later) that “it is not the responsibility of the class to worry about its construction”. You mean, in a dependency injection container context, that it is the container’s factory responsibility to create the objects? But how can this creation strategy affect to the assertion of non-null arguments in the constructor of a class?Best regards,JMB
Love the blog, and like other disagree with you on this case. If the class was dependant on abstractions and not concrete classes then testing would not be a problem. You could have the asserts and in your tests just use null objects or a mocking framework.
Because, I never want my entities in an invalid state, I’d prefer to keep the asserts in place and use a simple Object Mother (provide default stubs/mocks), if not a full on IoC, for testing.
Hi Miško,first, let me say that i really enjoy reading your blog!Back to topic:If you want to test Objects in an “inconsistent” state (i.e. not completely initialized), maybe setter injection would be a more flexible approach.
From experience, defensive programming has helped me avoid long debugging sessions on many occassions. I’m not sure I can agree with this post but it’s certainly food for thought.
I’ve been considering using the Findbugs annotations and perhaps the @NonNull annotation could be of use here.
With this annotation we can enforce the null check for client code under /src but not /test? -This could even have the added benefit of moving the constraint check from run-time to compile-time.
You say you can write a test for the factory that will asserts that non-null values are passed to the constructor. But how can you write such a test? I know Guice will take care of, but the contract of my factories is ‘you call create*(), I return an object’. The object collaborators are private field references and I cannot access them to see if they are null. Maybe a method verifyNull() on the created object can do the work, but to me it seems ugly.
Should we enforce constructor args?! « Journal of a software dev // Jun 27, 2009 at 3:54 am
[...] written and the information surrounding good OO and unit testing was, anyhow I came across this post which made me think about whether we should be asserting the collabarators in the constructor or [...]
Can you spot Java Puzzler in this snippet? at JAW Speak // Sep 30, 2009 at 1:00 pm
[...] and I saw this in a test with a particularly ugly constructor with 12 parameters. We were following Misko’s style of “use nulls in tests, but not in production code” and thus that Boolean [...]
I agree with previous commenters. Instead of: new LivingRoom(new Table(), new TV(), new Sofa()), we can write mock(LivingRoom.class). Replacing null with mock(Class) isn’t a hard task.
I would also strongly avoid nulls, as they make things ugly.
http://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare
Better use something like Null Object Pattern
Hm.. i’m having this problem right now. After some googeling i came here. Your article is quite good. In my opinion the House Object should not know anything about validation. You should be able to create a House without a kitchen, or even without roof. This means max. flexibility but also great danger. As some comments stated now it is possible to create a House Object which might not be in a consistence state. But the Object which has the responsibility to create a specific House Object has to perform these Validations.
I think what makes this a great article is all the comments. There’s a lot of disagreement, but also a lot of good ideas.
Here’s my thoughts, and I agree with JF and Craig M above.
Defensive programming is a good thing. Putting assertions in the ctor makes the class robust, and can save you “maalox” moments down the road. Until the time comes to move the assertions, then I think it’s safe to keep it there. A good question to keep in mind when designing is…”what is the purpose of class?”
If the purpose of the class is to never have NULLs, then it makes perfect sense to put the assertion in the ctor. However, if NULLs are allowable, then it’s best to “spread the defense” by moving the assertions to the secure() function.
void secure()
{
Assert.notNull(door);
Assert.notNull(window);
door.lock();
window.close();
}
“Early In, Early Out”
@kukudas,
it should be the responsibility of the house factory to make sure that the house is in the correct state. Also, your medium tests, should prove that it works as expected together.
@ josh,
such defense programing may come from C world where NullPointerExceptions are not handled well. In your example there is very little difference, if the program fails with assertion or with NPE a few lines down. If all other things were equal, I say defensive programing is a good thing, but things are not equal, since defensive programing makes it harder to write a tests. So the real question is, what is more valuable, defensive programing, which prove very little, or tests, which prove that your code works as expected. Of corse, it is not either or, and many times, you can have both defensive programing, and tests, but when they are at adds, I think testing should win.
Dependency Injection – Checking for null in the constructor | Greg Sochanik // Feb 4, 2012 at 9:40 am
[...] I’d like to go out with a quote from this post: [...]
Leave a Comment