Is using “new” in the constructor always bad?

Question

I have read that using "new" in a constructor (for any other objects than simple value ones) is bad practice as it makes unit testing impossible (as then those collaborators needs to be created too and cannot be mocked). As I am not really experienced in unit testing, I am trying to gather some rules that I will learn first. Also, is this is a rule that is generally valid, regardless of the language used?

Answer 1

There are always exceptions, and I take issue with the 'always' in the title, but yes, this guideline is generally valid, and also applies outside of the constructor as well.

Using new in a constructor violates the D in SOLID (dependency inversion principle). It makes your code hard to test because unit testing is all about isolation; it is hard to isolate class if it has concrete references.

It is not just about unit testing though. What if I want to point a repository to two different databases at once? The ability to pass in my own context allows me to instantiate two different repositories pointing to different locations.

Not using new in the constructor makes your code more flexible. This also applies to languages that may use constructs other than new for object initialization.

However, clearly, you need to use good judgment. There are plenty of times when it is fine to use new, or where it would be better not to, but you will not have negative consequences. At some point somewhere, new has to be called. Just be very careful about calling new inside a class that a lot of other classes depend on.

Doing something such as initializing an empty private collection in your constructor is fine, and injecting it would be absurd.

The more references a class has to it, the more careful you should be not to call new from within it.

Answer 2

While I'm in favor of using the constructor to merely initialize the new instance rather than to create multiple other objects, helper objects are ok, and you have to use your judgement as to whether something is such an internal helper or not.

If the class represents a collection, then it may have an internal helper array or list or hashset.  It would use new to create these helpers and it would be considered quite normal.  Such class will not offer injection to use different internal helpers, and has no reason to as these are hidden from the consuming client.  In this case, you want to test the object's public methods, which might go to accumulating, removing, and replacing elements in the collection.

---

In some sense, the class construct of a programming language is a mechanism for creating higher level abstractions, and we create such abstractions to bridge the gap between the problem domain and programming language primitives.  However, the class mechanism is just a tool; it varies by programming language, and, some domain abstractions, in some languages, simply require multiple objects at the level of the programming language.

In summary, you have to use some judgement whether the abstraction simply requires one or more internal/helper objects, while still being seen by the caller as a single abstraction, or, whether the other objects would be better exposed to the caller to create for control of dependencies, which would be suggested, for example, when the caller sees these other objects in using the class.

Answer 3

Not all collaborators are interesting enough to unit-test separately, you could (indirectly) test them through the hosting/instantiating class. This may not align with some people's idea of needing to test each class, each public method etc. especially when doing test after. When using TDD you may refactor out this 'collaborator' extracting out a class where it's already fully under test from your test first process.

Answer 4

As I am not really experienced in unit testing, I am trying to gather some rules that I will learn first.

Be careful learning "rules" for problems you've never encountered. If you come across some "rule" or "best practice", I would suggest finding a simple toy example of where this rule is "supposed" to be used, and trying to solve that problem yourself, ignoring what the "rule" says.

In this case, you could try to come up with 2 or 3 simple classes and some behaviours they should implement. Implement the classes in whatever way feels natural and write a unit test for each behaviour. Make a list of any problems you encountered, e.g. if you started off with things working one way, then had to go back and change it later; if you got confused about how things are supposed to fit together; if you got annoyed at writing boilerplate; etc.

Then try solving the same problem by following the "rule". Again, make a list of problems you encountered. Compare the lists, and think about which situations might be better when following the rule, and which might not.

---

As for your actual question, I tend to favour a ports and adapters approach, where we make a distinction between "core logic" and "services" (this is similar to distinguishing between pure functions and effectful procedures).

Core logic is all about calculating things "inside" the application, based on the problem domain. It might contain classes like User, Document, Order, Invoice, etc. It's fine to have core classes call new for other core classes, since they're "internal" implementation details. For example, creating an Order might also create an Invoice and a Document detailing what was ordered. There's no need to mock these during tests, because these are the actual things we want to test!

The ports and adapters are how the core logic interacts with the outside world. This is where things like Database, ConfigFile, EmailSender, etc. live. These are the things which make testing hard, so it's advisable to create these outside the core logic, and pass them in as needed (either with dependency injection, or as method arguments, etc.).

This way, the core logic (which is the application-specific part, where the important business logic lives, and is subject to the most churn) can be tested on its own, without having to care about databases, files, emails, etc. We can just pass in some example values and check that we get the right output values.

The ports and adapters can be tested separately, using mocks for the database, filesystem, etc. without having to care about the business logic. We can just pass in some example values and make sure they're being stored/read/sent/etc. appropriately.

Answer 5

Allow me to answer the question, gathering what I consider to be the key points here. I will quote some user for brevity.

There are always exceptions, but yes, this rule is generally valid, and also applies outside of the constructor as well.

Using new in a constructor violates the D in SOLID (dependency inversion principal). It makes your code hard to test because unit testing is all about isolation; it is hard to isolate class if it has concrete references.

-TheCatWhisperer-

Yes, using new inside constructors often leads to design flaws (for instance tight-coupling) which makes our design rigid. Hard to test yes, but not impossible. The property in play here is resilience (tolerance to changes)¹.

Nevertheless, the above quote is not always true. In some cases, there could be classes that are meant to be tightly coupled. David Arno has commented a couple.

There are of course exceptions where the class is an immutable value object, an implementation detail, etc. Where they are supposed to be tightly coupled.

-David Arno-

Exactly. Some classes (for example inner-classes) could be mere implementation details of the main class. These are meant to be tested alongside with the main class, and they are not necessarily replaceable or extendable.

Moreover, if our SOLID cult makes us extract these classes, we could be violating another good principle. The so-called Law of Demeter. Which, on the other hand, I find it to be really important from the design point of view.

So the likely answer, as usual, is depends. Using new inside constructors can be a bad practice. But not systematically always.

So, it takes us to evaluate whether the classes are implementation details (most of the cases they won't) of the main class. If they are, leave them alone. If they aren't, consider techniques like Composition Root or Dependency Injection by IoC Containers.

---

^{1: the main goal of SOLID is not to make our code more testable. It's to make our code more tolerant to changes. More flexible and in consequence, easier to test}

^{Note: David Arno, TheWhisperCat, I hope you don't mind I quoted you.}

Answer 6

As a simple example, consider the following pseudocode

class Foo {
  private:
     class Bar {}
     class Baz inherits Bar {}
     Bar myBar
  public:
     Foo(bool useBaz) { if (useBaz) myBar = new Baz else myBar = new Bar; }
}

Since the new is a pure implementation detail of Foo, and both Foo::Bar and Foo::Baz are part of Foo, when unit testing of Foo there is no point in mocking parts of Foo. You only mock the parts outside Foo when unit-testing Foo.

Answer 7

Yes, using 'new' in your application root classes is a code smell. It means you are locking the class into using a specific implementation, and will not be able to substitute another. Always opt for injecting the dependency into the constructor. That way not only will you be able to easily inject mocked dependencies during testing, but it will make your application much more flexible by allowing you to quickly substitute different implementations if needed.

EDIT: For downvoters - here's a link to a software development book flagging 'new' as a possible code smell: https://books.google.com/books?id=18SuDgAAQBAJ&lpg=PT169&dq=new%20keyword%20code%20smell&pg=PT169#v=onepage&q=new%20keyword%20code%20smell&f=false