Recently I worked my way through the “Effective Unit Testing” book. A nice read, nothing too complex or difficult, but it is always good to read the opinions of someone who really put some effort into a certain topic. One point was the question if code should contain comments or not, mainly because comments can have a negative impact because they can be incorrect and misleading. So the proposed solution was to write code that does not require comments at all; write “self documenting code”.
In my current project I run into similar situations on a daily basis; my idea on how code should look differs from the most of the other developers. I see people writing tons of code without a single comment in them and declare them “self documenting”. The problem is that for some reason I do not seem to be able to get comfortable with that code, and I’m really starting to wonder if I’m getting old, and am missing out on the “new and improved way” of doing things. But I’m not one to accept that without an effort, so I gave it a shot.
Below is an example of a simple unit test. Please do not spent too much time on this, because it is the “before” example. But the test is ok, there is nothing technically wrong with it, it just can be written better.
public void testVerifyGroupThenAddClient() throws ValidateException {
// group starts off not verified
GroupCare assertGroupCare = groupCareManager.findByObjectId(groupCare.getObjectId());
assertNull(assertGroupCare.getVerifiedAt());
// add client
groupCareManager.addClient(groupCare, client.getData(), groupActivity.getData(), user);
// group is not yet verified
assertGroupCare = groupCareManager.findByObjectId(groupCare.getObjectId());
assertNull(assertGroupCare.getVerifiedAt());
// verify the group
groupCareManager.verifyGroupCare(user, groupCare);
// assert that the group is verified
assertGroupCare = groupCareManager.findByObjectId(groupCare.getObjectId());
assertNotNull(assertGroupCare.getVerifiedAt());
// add new client
groupCareManager.addClient(groupCare, client2.getData(), groupActivity.getData(), user);
// group is no longer verified
assertGroupCare = groupCareManager.findByObjectId(groupCare.getObjectId());
assertNull(assertGroupCare.getVerifiedAt());
}
The next step was to apply the best practices according to the “Effective Unit Testing” book to it, which basically comes down to creating simple methods to be called in the test itself, and extract all scaffolding into helper methods. At first glance you will immediately see the improvement, and it most certainly is an improvement. However, now I want you to actually try and understand the test, actually read the code below. Humor me.
public void testIfAddingClientToVerifiedGroupMakesItUnverified2() throws ValidateException {
assertNotVerified(groupCare);
addClient(groupCare, client, groupActivity, user);
assertNotVerified(groupCare);
verify(groupCare, user);
assertVerified(groupCare);
addClient(groupCare, client2, groupActivity, user);
assertNotVerified(groupCare);
}
I’m very curious if you really have the feeling you understand what is tested in that code, but more importantly how much time and effort it took for you to understand it.
For me, I had been starting at that code for quite some time and it simply did not sit right. Yes, it is a huge improvement over the “before”, but I am used to write code differently: first write out what you intent to do (create a scenario), and then implement that. The big difference is that a scenario is written in a natural language, and natural languages are much easier to read for humans than computer code. And last time I checked (this morning under the shower) I still am human, and not a compiler.
So after I’d given that code a long stare, I decided it needed comments after all, which resulted in the code below. Again, please, read it and tell me which version is easier to comprehend.
public void testIfAddingClientToVerifiedGroupMakesItUnverified3() throws ValidateException {
// group starts off not verified
assertNotVerified(groupCare);
// when adding a client to the group, then it should stay not verified
addClient(groupCare, client, groupActivity, user);
assertNotVerified(groupCare);
// when verifying the group, then it should become verified (obviously)
verify(groupCare, user);
assertVerified(groupCare);
// when adding a second client to the group, then it should become not verified again
addClient(groupCare, client2, groupActivity, user);
assertNotVerified(groupCare);
}
So maybe I really am an old dump not being able to catch on to the latest way of writing code, but at least I confirmed that I’m still human 😉
Update:
Daan van Berkel has pointed out that using a fluent API both in the domain objects and the test library (Hamcrest) will make the example much more readable. And he is right, fluent API’s are a great improvement, as you can see below. The fact is that my current project does not have a fluent API, and fluent API’s can be really difficult to write correctly. Never the less, is the improved example below “self documenting”? Would you have understood what is tested just as easily as the example with the comments?
public void testIfAddingClientToVerifiedGroupMakesItUnverified2() throws ValidateException {
assertThat(groupCare, is(not(verified())));
groupCare.add(client.with(groupActivity)).as(user);
assertThat(groupCare, is(not(verified())));
groupCare.verify().as(user);
assertThat(groupCare, is(verified()));
groupCare.add(client2.with(groupActivity)).as(user);
assertThat(groupCare, is(not(verified())));
}
The issue here is not that comments are needed but that you are not using the “message” parameter in the assert methods.
// starts off not verified
assertThat(instance, is(not(verifiedInstance())));
instance.verify();
// verified after explicit verification
assertThat(instance, is(verifiedInstance()));
Is not what you want. Instead you want
assertThat(“starts off not verified”, instance, is(not(verifiedInstance())));
instance.verify();
assertThat(“verified after explicit verification”, instance, is(verifiedInstance()));
This has a double bonus as now the failing assertions tell you *why* the mis-match is a failure… Using comments and you need to go look in the code.
Having said that, a good comment is 10 times better than “self describing code” because it will tell you *why* the code is doing it this way… Bad comments just tell you what the code is doing and those are the ones that self documenting code should replace.
I never buy the comments can get out of date argument as method names can get out of date too…. And you may be constrained by having to maintain a backwards compatible API so self documenting code only works in the “private” scope.
I agree that a comment put to more effective use as the message parameter to the assert method, certainly is something worth considering. But in effect it still means that some sort of natural language addition to the code has value, as you describe. Thanks for pointing this out.
To conclude that comments are not needed is a step too far for me at this time; it still takes more time to interpret the code vs reading natural language. Testing frameworks like Spock also have natural language describing each step in the scenario. That said, my current project -as you know- does not have a fluent API and converting the code is next to impossible, so…
Hello Daan,
I always appreciate a good discussion, thanks for responding.
I’ve tried to incorporate your suggestions into a piece of code that does the same test. If I understand correctly, you would end up with something like this:
public void testGroupStartsOffNotVerified() throws ValidateException {
assertThat(groupCare, is(not(verified())));
}
public void testWhenAddingClientToGroupItShouldStayNotVerified() throws ValidateException {
groupCare.add(client.with(groupActivity)).as(user);
assertThat(groupCare, is(not(verified())));
}
public void testWhenVerifyingGroupItShouldBecomeVerified() throws ValidateException {
groupCare.add(client.with(groupActivity)).as(user);
verify(groupCare, user);
assertThat(groupCare, is(verified()));
}
public void testWhenAddingClientToVerifiedGroupItShouldBecomeUnverified() throws ValidateException {
groupCare.add(client.with(groupActivity)).as(user);
verify(groupCare, user);
groupCare.add(client2.with(groupActivity)).as(user);
assertThat(groupCare, is(not(verified())));
}
Correct?
Assuming this is correct, I totally agree that a fluent API greatly improves readability of code. We need more fluent AP’s! (BTW, the same can be said for named parameters… hm, I feel a new blog post coming up). But I’m not sure the splitting into four tests captures the scenario better. Furthermore the four tests have almost double the amount of code lines to, in essence, tests the same.
Anyhow, applying your code changes to my second example does improve readability:
public void testIfAddingClientToVerifiedGroupMakesItUnverified2() throws ValidateException {
assertThat(groupCare, is(not(verified())));
groupCare.add(client.with(groupActivity)).as(user);
assertThat(groupCare, is(not(verified())));
groupCare.verify().as(user);
assertThat(groupCare, is(verified()));
groupCare.add(client2.with(groupActivity)).as(user);
assertThat(groupCare, is(not(verified())));
}
So some code staring is required again to determine if I still want those comments.
I am glad to see that my point came across. In my opinion it is far more readable know and does not need comments.
Whether to split the test into multiple tests is worth an other discussion, but it would distract from the point of self documenting code.
At last, somenoe comes up with the “right” answer!
Hi Tom,
First things first, I do not think you are old. You are experienced and there is a difference.
Second, although you warned us that your blog is called ramblings on software development, I will respond to it because I think you have an interesting point, but are setting up a straw man.
The argument is made often when talking about self documenting code. Your arguments runs along the same lines. I.e. Here is some code that needs improving. Next I will show you code that is improved. But look, it still needs needs comments otherwise I can not follow it.
I can concur with you that the second code example is hard to decipher. I do not think this is because lack of comments. I think it is because the code does not expresses the intent very well.
In order to defend self documenting code, of which I am a great fan, I will try to improve upon the code as well. You do have me at an advantage, because I can not properly format the code. Nonetheless, I will valiantly will make my point.
First of all, in your first code example the test is called ‘testVerifyGroupThenAddClient’. The ‘Then’ signifies to me that you are testing more then one thing, which for me is a code smell in it self. I would rather have written multiple tests, e.g. one called ‘testGroupStartsOffNotVerified’ and ‘testWhenAddingClientToGroupItShouldStayNotVerified’ etcetera. Notice that I have taking your comments and turned them into method names. (I would even try to loose the ‘test’ in the method name if that was possible.)
In my opinion this reveals the intent of the test as clear as your comments document the intent.
So lets take a look at implementing the methods. Lets focus on ‘testGroupStartsOffNotVerified’. I would write an implementation that looks like the code below
assertThat(groupCare, is(not(verified())));
This perfectly reveals the intent of the test (in my opinion). This is not even dream code. Hamcrest makes it possible to write such code more or less verbatim.
One could argue that I have taken the simplest example and that the procedure of writing self documenting code would break down on a more complex example. I beck to differ, unfortunately the original test code is lacking intent as well. So I will go out on a limb and will ‘dream up’ a better self documenting code for adding a client (with, apparantly, a certain groupActivity).
If I did not knew the API I would want to write
groupCare.add(client.with(groupActivity));
Again, the code tells my that a client with a certain groupActivity is added to the specific groupCare. (I do not know the specifics of the domain, so the example could be improved by talking to a domain expert.)
So I still think that the bulk of the comments that are written could be replaced by self documenting code.