37

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?

  • 9
    It doesn't make testing impossible. It does make maintaining and testing your code harder, though. Have a read of new is glue for example. – David Arno Feb 1 '18 at 15:27
  • 38
    "always" is always incorrect. :) There are best practices, but exceptions abound. – Paul Feb 1 '18 at 15:51
  • 63
    What language is this? new means different things in different languages. – user2357112 supports Monica Feb 1 '18 at 20:22
  • 13
    This question depends a bit on the language, doesn't it? Not all languages even have a new keyword. What languages are you asking about? – Bryan Oakley Feb 1 '18 at 22:06
  • 7
    Silly rule. Failing to use dependency injection when you should has very little to do with the "new" keyword. Instead of saying that "using new is a problem if you use it to break dependency inversion", just say "don't break dependency inversion". – Matt Timmermans Feb 2 '18 at 3:44
36

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.

  • 12
    I really don't see any value in this rule. There are rules and guidelines on how to avoid tight coupling in code. This rule seems to deal with the exact same issue, except that it's incredibly too restrictive while not giving us any additional value. So what's the point? Why would creating a dummy header object for my LinkedList implementation instead of dealing with all those special cases that come from having head = null in any way improve the code? Why would leaving included collections as null and creating them on demand be better than doing so in the constructor? – Voo Feb 1 '18 at 20:54
  • 22
    You're missing the point. Yes a persistence module is absolutely something a higher level module should not depend upon. But "never use new" does not follow from "some things should be injected and not directly coupled". If you take your trusty copy of Agile Software Development you'll see that Martin is generally talking about modules not single classes: "The high level modules deal with the high level policies of the application. These policies generally care little about the details that implement them." – Voo Feb 1 '18 at 21:15
  • 11
    So the point is that you should avoid dependencies between modules - particularly between modules of different abstraction levels. But a module can be more than a single class and there's simply no point erecting interfaces between tightly coupled code of the same abstraction layer. In a well designed application that follows SOLID principles not every single class should implement an interface and not every constructor should always only take interfaces as parameters. – Voo Feb 1 '18 at 21:17
  • 18
    I downvoted because it's a lot of buzzword soup and not much discussion about practical considerations. There's something about a repo to two DBs, but honestly, that's not a real world example. – jpmc26 Feb 1 '18 at 23:00
  • 5
    @jpmc26, BJoBnh Without going into if I condone this answer or not, the point is expressed very clearly. If you think "dependency inversion principal", "concrete reference" or "object initialization" are just buzzwords, then actually you just don't know what they mean. Thats not the answer's fault. – R. Schmitz Feb 2 '18 at 10:20
50

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. The class does not offer injection to use different internal helpers and has no reason to. 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.

  • 4
    +1. This is exactly right. You have to identify what the class' intended behavior, and that determines which implementation details need to be exposed and which don't. There's a sort of subtle intuition game you have to play with determining what the class' "responsibility" is, too. – jpmc26 Feb 4 '18 at 10:01
27

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.

  • 14
    Not all collaborators are interesting enough to unit-test separately end of story :-) , This case is possible and nobody dared to mention. @Joppe I encourage you to elaborate the answer a little bit. For example, you could add some examples of classes that are mere implementation details (not suitable for replacement) and how they can be extracted latter if we deem it necessary. – Laiv Feb 1 '18 at 15:46
  • @Laiv domain models are typically concrete, non-abstract, you don't go about injecting nested objects there. Simple value object/complex object having otherwise no logic are also a candidate. – Joppe Feb 1 '18 at 16:12
  • 4
    +1, absolutely. If a language is set up so that you have to call new File() to do anything file-related, there's no sense in forbidding that call. What are you going to do, write regression tests against the stdlib's File module? Not likely. On the other hand, calling new on a self-invented class is more dubious. – Kilian Foth Feb 1 '18 at 16:19
  • 7
    @KilianFoth: Good luck unit testing anything which directly calls new File(), though. – Phoshi Feb 1 '18 at 16:36
  • 1
    That is a guesswork. We can find cases where it does not make sense, cases where it's not useful and cases where it make sense and it's useful. It's a matter of needs and preferences. – Laiv Feb 1 '18 at 16:41
13

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.

6

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)1.

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.

3

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.

-3

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

  • 15
    Yes, using 'new' in your non-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. Why is this a problem? Not every single dependency in a dependency tree should be open to replacement – Laiv Feb 1 '18 at 15:40
  • 4
    @Paul: Having a default implementation means that you take a tightly-bound reference to the concrete class specified as the default. That doesn't make it a so-called "code smell," though. – Robert Harvey Feb 1 '18 at 15:58
  • 8
    @EzoelaVacca: I would be cautious about using the words "code smell" in any context. It's kinda like saying "republican" or "democrat;" what do those words even mean? As soon as you give something a label like that, you stop thinking about the real issues, and learning stops. – Robert Harvey Feb 1 '18 at 16:01
  • 4
    "More flexible" is not automatically "better". – whatsisname Feb 1 '18 at 17:20
  • 4
    @EzoelaVacca: Using the new keyword is not a bad practice, and never was. It's how you use the tool that matters. You don't use a sledgehammer, for example, where a ball peen hammer will suffice. – Robert Harvey Feb 1 '18 at 17:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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