Wednesday, 27 March 2013

Moq As<> a code smell

The moq As method allows you to create a mock object that implements more than one interface:

(If IFirstInterface implements ISecondInterface then the As<> is not required.)

Its easy to see how this could help you write a test if the SUT cast the object of type IFirstInterface to ISecondInterface.

But what does this say about the code?

It's saying that you have a class that needs to implement more than one interface in order to performs it's job correctly. When you have a class that implements more than one interface, it suggests it has more than use, more than one reason to change. The need to use As<> is suggesting a Single Responsibility Principle violation.

While I wouldn't expect to see As<> used in tests for a Domain Model or Business Logic, it is possibly useful in "plumbing" layers, such as when adapting an external service. As always with code smells, use your own judgement.

To find a bug.

So I thought as I've suggested that debuggers are bad I should describe my "process" for tracking down bugs. I'm not going to claim this is as perfect approach, but it seems to work for me.

What is the bug?

The first thing I try and do when receiving a bug report is get information on what the bug actually is. What the expected behaviour was and what the actual the behaviour was. Any experienced developer will at some point have misunderstood an issue and fixed the wrong problem. This step is vital, but not really what I want to talk about in this post.

Where is the bug?

Time to start theorising. What components are likely to be involved with the operation? This may be an easy question to answer if you know the code well, but if you don't, try doing a search for keywords in your code this is where good naming (and also good spelling) can really make things easier. (I think there's another post in that statement).

Now the most important step of all - read the code!

When I read code, knowing there is an issue with it, I gain a new perspective on it. I notice things I didn't see before and question the assumptions that it makes. I start to theorize about how this could possibly be doing what it is actually doing. Tests are important here as well, is there a test for the behaviour seen? If there is, then maybe the behaviour is by design, or maybe there are situations that the tests don't account for. The amount that can be learnt from this type of inspection goes far beyond that of a normal code review, where you see what you expect to see rather than what is actually there.

You can also learn a lot about how to write code from this. If a particular pattern is hard to understand this will become very obvious at this stage, and you can consider other patterns in the future. You can also spot what is a good name and what isn't. At this stage if the code is quite complicated I will often refactor it. This is particularly useful if there are no unit tests as refactoring will require you to write some.

I'd say the majority of bugs I look at will be found and understood by this point, usually much more quickly than it would take to fire up a debugger. Plus my understanding of the code will be greatly improved.

If I still haven't figured it out, now is the time to fire up the debugger. But this is made much easier due to my increased understanding of the code. I know where to put break points and have an idea of what I should be seeing.

Where else could this bug exist?

It always surprises me how often the same bug occurs more than once within a codebase. Patterns of usage tend to be copied and reused. The most obvious benefit of looking for other occurrences of a bug is that you may uncover another bug in your software, but it also gives you the opportunity to increase your understanding of the code and possibly improve it. Looking for these bugs you may find alternative approaches to the problem. If the bug is in business logic you may find DRY principle violations, or you may find ways of simplifying the problem so all places can be cleaned up and improved.

How do we reproduce it?

Write a test. This could be a unit test, an integration test or an acceptance test, which one really depends on the bug. Unit tests are light, and easy to implement, but acceptance test have the advantage that they are easier to show to the user and get them confirm this is the behaviour they expected.

Fix, review, push the changes.

Go home.

OK, now I've finally got that post out of the way, I can get on to the interesting stuff - how to make this process easy.