CODEWITHSUNDEEP
javaunit-testingconstructor
Busch
Busch

Reputation: 959

Is creating a private constructor for testing bad practice?

I have encountered some java code where the public constructor calls a package-private constructor with a bunch of new operators to create new object.

public class Thing {

    //public
    public Thing(String param1, int paramm2) {
        this(param1, param2, new Dependency1(), new Dependency2());
    }

    //package-private for the sake of testing
    Thing(String param1, int param2, Dependency1 param3, Dependency2 param4) {
        this.memberVar1 = param1;
        this.memberVar2 = param2;
        this.memberVar3 = param3;
        this.memberVar4 = param4;
    }


    //...rest of class...
}

In my opinion, this is wrong because you are writing code to test it rather than writing correct code. I suppose the other two options (that I can think of) would be to create a factory or to use PowerMockito to inject a new object where applicable. Personally, I would have written as shown below. 

public class Thing {

    //public
    public Thing(String param1, int paramm2) {
        this.memberVar1 = param1;
        this.memberVar2 = param2;
        this.memberVar3 = new Dependency1();
        this.memberVar4 = new Dependency2();
    }

    //...rest of class...
}

What is the best practice/correct way to implement this?

Upvotes: 8

Views: 6746

Answers (2)

jaco0646
jaco0646

Reputation: 17104

I know there are good discussions of this exact topic elsewhere on StackExchange, but my luck with Google is not strong right now. The duplicates I've found are less than enlightening.

Personally, I've seen three types of test code in production.

  1. Branching logic - if (isTesting) foo() else bar();
  2. Test method - Object foo(); is called by testing code but never in production.
  3. Test constructor - Foo(Object dependency1, Object dependency2) is called by testing code but never in production.

The first two types are harmful in several ways.

  • Readability suffers because the code is longer and more complex. Business logic can be obscured by testing logic.
  • Maintainability suffers because the code has more responsibilities. Updating tests may require changes to production code.
  • In the best scenario, this test code is dead code in production. In the worst case, a client accidentally executes this test code in production causing unpredictable results.

The third type can of course be harmful in the same ways listed above; however, I believe a so-called test constructor can mitigate or eliminate the problems inherent to other types of test code in production.

  • Adding a constructor obviously lengthens the code (at least slightly) but it need not add complexity: if the constructors are chained, as in the example here, we have one which instantiates defaults and another which simply assigns fields. This separation can be less complex than a single constructor which does both. And if we follow the commonly accepted practice of not implementing business logic in our constructors, then we should have little risk of obscuring it.
  • Assuming, again, that our "test" constructor is chained to a production constructor and implements no logic other than assignment, the code actually hasn't added a responsibility to the class: it still instantiates and assigns dependencies, but as separate methods rather than one. There is no chance of a testing change breaking our production code.
  • Under the assumptions outlined, our test constructor is not dead code: it is actually called in production from whichever chained constructor the client invokes; therefore, a client executing this second constructor (with appropriate arguments) could expect the same contract as from the chained constructor.

Invoking new inside a constructor is generally bad for the same reasons that dependency injection is generally good. However, there are scenarios where sensible default implementations of a class's dependencies are obvious and instantiating those dependencies in a constructor provides the simplest possible interface to clients. In these scenarios, I find it appropriate to chain a second constructor which does not force any defaults, regardless of whether the second constructor facilitates testing.

As a bonus, the chained-constructor practice encourages programming to an interface because the second constructor makes you think about dependency implementations other than the defaults.

Upvotes: 2

CodeBlind
CodeBlind

Reputation: 4569

It is generally bad form to include test-specific code in anything that gets released (but there are exceptions, so don't read too closely). Here's a couple reasons why.

1. Someone external might use the test constructor in a way you never intended, hosing up everything, because they either didn't read the docs indicating it's meant for testing or the developer forgot to document it.

Let's say you wanted to write some useful extension of Thing but you couldn't find anything in Thing's public/protected API that did what you wanted it to. Then you find this package-private constructor that seems to allow what you were hoping for, only to find out later that it breaks your code. You still can't do what you wanted to and you wasted time exploring part of the API that didn't pan out. Anyone who does this will be left with a negative opinion of the API and won't be likely to recommend it to anyone else.

2. Refactoring the package name will break stuff.

This test code isn't very resilient to refactorings happening in the production code because of the way Java's default visibility works. The test code can only call that constructor if it resides in the same package. If the package gets renamed, the test code calling it won't be able to access it, causing compile errors. Sure, it's an easy fix for whoever is developing the code and the tests, but refactorings already aren't fun even without adding this minor annoyance. It becomes a major problem if a bunch of people were previously able to use the package-private stuff to suit their needs successfully - now all their code is broken too.

There are definitely cases where it's difficult write code that can be run in both the test and production environments (e.g. functions that only run when your application is networked). In those cases, dependency injection can be your friend, but simpler tests are always best if more complicated testing schemes can be avoided without sacrificing feature coverage or adding hooks into your API you never intended other developers to see.

Upvotes: 7

Related Questions