Take a look at this code:
1: public class ClassToTest {
2:
3: public void MethodToTest()
4: {
5:
6: IDAL dal = new DAL();
7:
8: int discount = dal.GetDiscount();
9:
10: if (discount > 10)
11: {
12: // Do something
13: }
14:
15: }
16: }
The tight coupling to DAL just stands out like a sore thumb. One consequence of this tight coupling is that it's hard to test if we don't have an underlying database setup with correct values. Forget about testing this while taking a shower, unless you have a habit of taking your SQL Server box to the bath tub with you.
Take a look at the following approach to making this code testable without having a database:
1: public class ClassToTest {
2:
3: public void MethodToTest()
4: {
5:
6: DAL dal = new DAL();
7:
8: #if testing_in_shower
9: int discount = 3;
10: #else
11: int discount = dal.GetDiscount();
12: #endif
13: if (discount > 10)
14: {
15: // Do something
16: }
17:
18: }
19: }
Are you still here? Didn't you just fall off of your chair, not knowing whether to laugh or cry in anguish? It's just plain wrong isn't it? Code Smell just takes on a whole new dimension (and consistency?). Something like this shouldn't be done for many reasons. It makes our code testable but it's wrong.
Introducing code for testing purposes
The reason I bring this up now is because in the past 2 weeks I've seen a few references and examples of introducing code just for the sake of testability. Two weeks ago I was skimming over an article that was explaining unit tests. The class the author was trying to test had a dependency that was causing issues during his tests. The solution offered was to introduce a new constructor and inject the dependency in. In our case, the ClassToTest would like:
1: public class ClassToTest {
2:
3: public ClassToTest()
4: {
5:
6: // Initialize some stuff
7:
8: }
9:
10: public ClassToTest(IDAL dal)
11: {
12: // Initialize some stuff
13: _dal = dal;
14: }
15:
16: ....
Now that's great. The problem is that the author left both constructors in there, emphasizing that one is for testing purposes and the other for production code. Why that's wrong I'll get to in a moment.
The other day, Scott Hansleman made a blog post on testing some code that had a dependency on the current principal. His solution was to inject IPrincipal into various methods of the Controller, where Controller is in the context of an ASP.NET MVC Controller. Personally I don't agree with the solution but that's another subject (and another post on a different approach I have in the backlog for the same scenario). However, injecting the dependency seemed valid enough. Later on he updated the post with the following:
UPDATE: Phil had an interesting idea. He said, why not make method overloads, one for testing and one for without. I can see how this might be controversial, but it's very pragmatic.
- [Authorize]
- public ActionResult Edit(int id)
- {
- return Edit(id, User); //This one uses HttpContext
- }
[Authorize] public ActionResult Edit(int id) { return Edit(id, User); //This one uses HttpContext }You'd use this one as before at runtime, and call the overload that takes the IPrincipal explicitly for testing.
The same thing has happened as in the case of an overloaded constructor. Code has been introduced into the base just for the sake of testability. In the previous case it was a new constructor, and in this case, a new method.
Why is this wrong?
Introducing code just so that you can test it is wrong for several reasons, including an increase in noise, decrease in readability and more importantly, permitting the possibility of production code that will not be tested. It might seem improbable at first, seeing that the overloaded versions always call the base ones, but as your code base grows, by allowing this habit, you can promote it to other structures such as conditionals, switch statements, etc, and not limit it only to methods or constructors. Then you'll start initializing values that only make sense in tests and gradually your code starts to smell more and more. To top it off, you can throw in some compiler conditional that rips those parts of the code out from the production version.
Code should also serve as documentation. The cleaner you make your code, the less noise and redundancy you add, the easier it is for others to understand and maintain.
Conclusion
Being pragmatic is good, but don't do it at any cost. Try and not introduce code into your code base that is there exclusively for the purpose of testability (and no, dependency injection is not only for testability). Anything that has to do with testing, keep it where it should be, in the test assemblies. Having overloaded methods or constructors, mocks or stubs in your code base is just as bad as having compiler conditionals in your code for testability. If you run into issue where your code is hard to test, step back and re-think your design. Maybe the problem you're facing now that's impeding your tests is actually a can of worms waiting to be opened. And the sooner you open it, the better. That's one of the great advantages of Test Driven Development.