11

The linked "duplicate" question is an iffy match at best, because it's asking

  • is pattern X OK (YES/NO)

and I'm clearly already in the NO camp, and subsequently asking

  • what is pattern X called
  • 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
  • We could evaluate ISomething better if we knew a little more about its design. Can it be used with many classes, or just this class? Does it have a sensible name? Nov 20, 2014 at 16:45
  • 2
    I respectfully disagree. Notice that MyClass has no interface. Thus MyMethod should never be called (in production) with an implementation of ISomething other than Something - yet the overloaded constructor allows for this (invalid unless in a test context) possibility.
    – Michael
    Nov 20, 2014 at 17:05
  • 5
    Trying to cram a book of theory into a comment - If someone came to me with this for code review I would argue that the empty constructor is the one which shouldn't exist - consider removing this one and ALWAYS passing in an ISomething (either through code or an IoC mechanism)
    – Liath
    Nov 21, 2014 at 8:26
  • 2
    @Michael: your question is at risk of beeing closed, because it is in fact a mixture of two things - first, the question "what is the name of this ...", and second, some kind of rant against this particular solution which you obviously don't like. If you remove the rant part, chances are much higher that your question won't be closed.
    – Doc Brown
    Nov 21, 2014 at 16:35
  • 1
    Suggestion for getting the question reopened: Forget about what the name of the problem is. It really doesn't matter. Clearly describe the code that you have and what it is doing (you don't need the actual code, but a medium level description of it). Anti-patterns exist because they attempt to fix a problem, but do so in a way that ultimately costs more than leaving the problem as is. Describe the problem you are having with the code and what you ultimately want the outcome to be. That is a solvable problem, but you need to describe the problem and issues you are having with the design.
    – user40980
    Nov 24, 2014 at 23:59

7 Answers 7

26

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.

16

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
  • I'm not sure I understand what you're getting at here. Classes with parameterless constructors don't have any dependencies, and the OP is discussing constructors that are only used for testing. Typically such constructors allow testing in the absence of the usual dependencies. Nov 20, 2014 at 16:36
  • 9
    @RobertHarvey - This class needs an ISomething to work, hence a dependency. The parameterless constructor in the example is filling that dependency with an assumption.
    – Telastyn
    Nov 20, 2014 at 16:38
  • 5
    @RobertHarvey: I agree with Telastyn: in the shown example, the test-only constructor should probably not for "test-only", and the parameterless constructor shoud probably not depend on Something. But maybe just this specific example is flawed.
    – Doc Brown
    Nov 20, 2014 at 16:40
  • Well, if what you're saying is that this particular constructor example doesn't necessarily have to be confined to unit testing, I'm inclined to agree with you, though it might be useless in practice (depending on what the class actually does). Nov 20, 2014 at 16:41
  • 3
    @Telastyn Indeed, that's basically the entire principle behind the idea that TDD is a useful design method. Changes made to improve testability usually lead to clearer & more flexible code in the long run. Delete the no-args constructor and centralize initialization somewhere so dependencies are captured at a higher level of abstraction than implementation. The resulting design is almost always better.
    – Jules
    Nov 21, 2014 at 8:41
12

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
  • For testing file IO, IMO it's generally simpler just to accept that the WriteFile() and ReadFile() methods of the class will only be covered by relatively lightweight integration tests that do actual file io instead of trying to mock the file system itself. Nov 20, 2014 at 22:21
  • 1
    In .NET, I/O of all kinds is abstracted via Stream objects. So you can replace the FileStream with a pre-populated MemoryStream for testing purposes. Nov 21, 2014 at 3:26
7

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
  • 4
    (And personally, I think "Test-Driven Damage" would have been pithier!) Nov 20, 2014 at 16:33
  • this reads more like a comment, see How to Answer
    – gnat
    Nov 20, 2014 at 16:33
  • @gnat Thanks. I'm not sure what more needs to be said as an answer to "is there a name for this" than what the name is. Nov 20, 2014 at 16:33
  • "Read the question carefully. What, specifically, is the question asking for?" (How to Answer). You seem to be addressing only what is in the title, while question asks: "what can be done to solve it?" FWIW even the title asks about "Fix"
    – gnat
    Nov 20, 2014 at 16:37
  • @gnat I don't know what can be done to solve it, I only know what it's called. It seems more like the problem you have is that the questioner has asked two different questions. Because I do know that we're not supposed to answer questions in comments. Nov 20, 2014 at 16:39
3

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
  • 1
    I'll admit my initial example wasn't the best at illustrating my point, but it's really irrelevant to the overall question (which, unfortunately, dragged IoC into the conversation). The question could have just as easily been asked as something like what can be done about method X that exposes internals improperly for testing?
    – Michael
    Nov 21, 2014 at 14:49
  • I understand your frustration that you feel your question has been derailed. As I said in my answer, "such code would normally be a smell indicating that something's up with the shape of your class' public signature". In this case, the problem is that your class is instantiating its own dependency. In other cases, it may be something else. You're essentially asking "What is it called when a poorly designed class is hard to test. What can be done about it [other than fixing the poor design]?" Nov 24, 2014 at 10:29
0

Two possible cases when you may want to change your production code for testing purposes:

  1. If you have complex functionality in the class
    1. And your open interface does several things combined
    2. so you can not test each of the functionality separately
    3. this means when a combined test fails you still don't know which part is broken
    4. YES, it is good to separate that functionality into subclasses and test them
  2. If you want to test internal implementation
    1. This brakes the refactoring/encapsulation principles because
    2. your business logic will not brake with new implementation
    3. 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.

-2

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
  • 2
    Adding something now for flexibility that might someday be useful is a bad idea. YAGNI.
    – Andy
    Nov 22, 2014 at 1:16
  • And why does this mean making the constructor deprecated is a bad solution?
    – Jules
    Nov 22, 2014 at 8:14
  • Because it mat be a better solution to not have an ISomething at all. You're advocating adding complexly because you might need more flexibility. Coding for mights typically creates a architecture worse than if you never introduced the ISomething, and it makes changes in the future which you actually need much more difficult.
    – Andy
    Nov 23, 2014 at 17:31
  • I'm not advocating that. I'm suggesting that that is a reasonably common point of view, and then providing a potential solution for those who disagree. So, again, why does the fact that I quote an opinion you disagree with before stating an answer intended specifically for those who disagree with that opinion reflect negatively on the answer as a whole?
    – Jules
    Nov 24, 2014 at 9:56

Not the answer you're looking for? Browse other questions tagged or ask your own question.