Is creating a private constructor for testing bad practice?

Question

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?

Answer 1

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.