The linked “duplicate” question is an iffy match at best, because it’s asking
- is
pattern XOK (YES/NO)
and I’m clearly already in the NO camp, and subsequently asking
- what is
pattern Xcalled - what steps can be taken to fix
pattern X
(neither of which are addressed by the linked question).
I recently did a code review on a block of code that looked something like this:
public class MyClass
{
private ISomething mySomething;
// ...Other variables omitted for brevity
public MyClass() { mySomething = new Something(); }
/// <summary>
/// Constructor - ONLY USE THIS FOR UNIT TESTING
/// </summary>
public MyClass(ISomething something) { mySomething = something; }
public void MyMethod()
{
// Gets called by the framework, and changes the internal state of the class by using mySomething...
}
// Other methods...
}
I’m concerned specifically with the overloaded constructor. It was added purely to test this class, and will make its way into production code.
Is there a name for this pattern/anti-pattern, and what can be done to solve it?
For clarification, the implementation of Something was added specifically for the purpose of being able to add an overloaded constructor to MyClass. It’s used nowhere else. Its existence is an instance of the very issue I’m concerned about.
ISomething is very tightly coupled to MyClass. It needn’t have been extracted. Implementation and interface might as well look like:
public interface ISomething
{
string GetClassName();
}
public class Something : ISomething
{
public string GetClassName() { return "MyClass"; }
}
That means that MyClass.MyMethod()‘s body could just be replaced with return "MyClass";
However, the interface abuse/premature optimization seems like a separate issue and not in the spirit of the original question (i.e. consider it a given that the class/interface is structured like so and leave it as a separate [but valid] concern).
16
For methods of a class which are solely for testing purposes, I have seen the name maintenance hatch in the past. And similar to real maintenance hatches in physical machines, those methods sometimes have their purpose. For example, if you are going to make some legacy code testable when it has grown too big after some years of evolving, maintenance hatches can be of great value.
But I also agree to the other answers here, such methods should be an exception, and when you keep classes and components small, with well designed interfaces, you seldom need them.
It was added purely to test this class, and will make its way into production code.
This is shortsighted…
Having a constructor to pass in dependencies isn’t done just to test the class. It’s done to make your class flexible. The parameterless constructor that has a hard dependency on a concrete Something is more of the anti-pattern due to the tight coupling, and is added only for programmer convenience.
6
It’s called “production code included for the sole purpose of facilitating testing.”
If you’re using it a lot, I’d say it’s an anti-pattern. The way you mitigate it is to write your classes using dependencies which conform to an Interface and are supplied using Dependency Injection, and then use stubs and mocks to isolate the class for testing.
But the reality is that it’s hard to isolate some classes. Despite its reputation for testability, ASP.NET MVC has some features in it like HttpContext that are notoriously difficult to mock. Classes that are designed to perform file handling can be difficult to test without helper code, because you have to set up elaborate file and folder scenarios to test them. So I can see legitimate reasons for including such code.
2
One term which has been used for this recently is test induced design damage.
Telastyn is correct that your code snippet isn’t actually a very good example of this concept, though.
A more common occurence is that a test wants to set up a class to be in a particular state during its “arrange” phase, but due to encapsulation, doesn’t have any way to do that directly. So the test has to choose either to execute a series of steps to get the object to the desired state (which requires coupling the test to much more than just the particular statement it’s trying to verify), or to break encapsulation. Choosing the latter option requires potentially-dangerous modifications to production code.
6
There isn’t, so far as I’m aware, a general name for production code created specifically for the purpose of unit tests. With the notable exception of friend-assemblies (for unit-testing internal units) such code would normally be a smell indicating that something’s up with the shape of your class’ public signature.
This particular example, however, does have a specific name. It is called “Poor Man’s Dependency Injection” (or sometimes “Bastard Injection”) because of the way it looks like proper dependency injection but doesn’t actually break the coupling between the caller and the dependency.
The anti-pattern here isn’t:-
public MyClass(ISomething something) { mySomething = something; }
The anti-pattern here is:-
public MyClass() { mySomething = new Something(); }
Now, it’s not necessarily the case that anything needs fixed here immediately. You might be ok to let it slide this once, but if it comes up often you’ll want to remove the parameterless constructor and have MyClass always receive its dependency from external code – possibly with the aid of an IoC Container or ServiceLocator.
(Note: While IoC Containers are much more fashionable than ServiceLocators these days, the important thing is that you separate configuration and usage. Using either is better than using neither.)
Responding to your edit:-
As I said above, testing difficulties are normally indicative of poor design. You can fix that in two ways. Either you can open up some doors for test code to (inappropriately, as you put it) access the internals of an otherwise impossible-to-test object, or you fix the underlying design problem.
Now, that’s a bit strong, because the correlation between “well-designed code” and “code that is easy to test” isn’t quite 100%. In the case you’ve called out in your OP, the design problem looks obvious (Object instantiates its own dependency -> DIP violation, although on closer inspection it may also be the case that the dependency is unnecessary). Sometimes the design problem is less obvious, or – especially in legacy code where architectural refactoring is difficult and risky – not worth solving. You’ll need to use your judgement to figure out which is which.
2
Two possible cases when you may want to change your production code for testing purposes:
- If you have complex functionality in the class
- And your open interface does several things combined
- so you can not test each of the functionality separately
- this means when a combined test fails you still don’t know which part is broken
- YES, it is good to separate that functionality into subclasses and test them
- If you want to test internal implementation
- This brakes the refactoring/encapsulation principles because
- your business logic will not brake with new implementation
- but your tests will brake, which is BAD since tests should verify object behavior from its user perspective.
So the answer is that the anti-pattern (item 2) logically breaks intention and purpose of encapsulation. I vote for the violation of encapsulation principle.
Assuming you do not accept the argument (which I believe is well worth considering) that the fact that the separated ISomething is required for testing tells you that it may also be required kn future to make your class more flexible, one possible fix would be to mark the constructor as deprecated and disable deprecation warnings for the line of the test class that uses it. This seems much better than internal, as that does little to prevent actual use.
4