views:

220

answers:

10

When creating methods, should every object instantiated inside that method be passed in as a parameter so that those objects can be mocked in our unit tests?

We have a lot of methods here at work that have no associated unit tests and upon writing tests retrospectively; we find that there are quite a lot of objects instantiated inside these methods.

One of our options is to refactor our current methods into more unit like methods and reduce the number of responsibilities per methods. This could be quite a lengthy process but most certainly be a large benefit to us in the future.

What do you think? Should all objects instantiated inside a method be passed in as parameters?

+6  A: 

Maybe not all objects, but the more objects you inject into your unit, the better your separation of concerns tend to become, so I would definitely recommend that you move in that direction.

You don't have to pass all objects as method parameters. It is often a better design to inject collaborators into classes via Constructor Injection. This keeps your interfaces clean while your implementations can import the collaborators it needs.

Let's say that your original implementation looked like this:

public class Foo
{
    public Ploeh DoStuff(Fnaah f)
    {
        var bar = new Bar();
        return bar.DoIt(f);
    }
}

This could be changed to look like this:

public class Foo
{
    private readonly IBar bar;

    public Foo(IBar bar)
    {
        this.bar = bar;
    }

    public Ploeh DoStuff(Fnaah f)
    {
        return this.bar.DoIt(f);
    }
}

Notice that I changed bar from an instance of Bar to an instance of IBar, thus decoupling Foo from the specific implementation of IBar. Such a refactoring tend to make your unit tests simpler to write and maintain, since you can now vary the implementations of Foo and Bar independently.

Mark Seemann
+5  A: 

The first part is a bit of a loaded question. It's like asking "When running over pedestrians in my car, should I have both hands on the wheel?"

A method that is instantiating lots of other objects is almost certainly doing too much. A class with lots of those methods is probably not following the single responsibility principle.

However, one of the key ways to make your code testable is to use IoC (inversion of control), where a class's dependencies (or a method's) are passed to it, rather than the class asking for them. This makes it much easier to test, as you can pass in mocks.

So the short answer is "Yes", pass in your dependencies and look at a good dependency injection component. The long answer is "Yes, but don't do that". DI frameworks will probably force you to pass in dependencies to objects rather than methods, and you'll find you want to make sure you limit those dependencies - which is a good thing.

And certainly, refactoring to reduce dependencies is good. Shortening your methods to do one thing is almost never bad. I would strongly agree this is a long term gain, as long as you can afford the short term costs.

Philip Rieck
+1 for the analogy. It's Monday and I needed a good laugh.
wheaties
A: 

At some point you are not going to get away from creating from one object from another but you should be writing software to good design principles. E.g. SRP , DI etc..

Where you have many dependencies you may fund an IoC container will help you to manage them all.

When dealing with legacy code, you may find it useful to read Micael Feather's Working Effectively with Legacy code. The book has many tehcniques on how to get your system under test.

John Nolan
A: 

I'm not sure what language/tools you're using, but the way it can be done it rails is simply to mock the constructor, like so:

@user = mock_model(User)
User.stub(:create).and_return(@user)

So from now on in your tests if you call User.create (User is the "Class"), it will always return your predefined @user variable, allowing you total control of what data is being used in your unit tests.

Now that you know exactly what data is in your tests, you can begin to stub the instance methods of your objects to make sure that they're returning proper data that you can use to test your methods.

Mike Trpcic
A: 

You should be favouring methods that take no arguments, followed by one argument, two and finally three. Anything taking more than 3 is a code smell. Either there's a class waiting to be discovered in all the arguments that are getting passed in or the class/method is trying to do too much.

In terms of passing in dependencies, you can use constructor injection but over time this gets unwieldy as you slowly move to having to pass in the entire object graph. My advice would be to move to using an IoC container sooner rather than later and avoid the pain.

FinnNk
+1  A: 

Just an observation: you are talking about methods, while I prefer talking about classes.

There is no general answer to this question. Sometimes it is really important to decouple the creation of a class from the usage.

Consider:

  • if a class uses another, but you want to loosely couple them, you should use an interface. Now, if only the interface is known, not the concrete type, how should the first class create an instance?
  • decoupling classes is very important, but can't and shouldn't be done in every case. If in doubt, if should be decoupled.
  • When using injection, you need to decide who is creating and injecting instances. You will most probably find a dependency injection framework like Spring.Net come in handy.

There is another trick: make injection optional. This means, when you pass an instance in the constructor, it will be used. When not, a new instance is created by the class itself. This is handy when dealing with legacy code and when you don't have an dependency injection framework.

Stefan Steinegger
+1  A: 

[Disclaimer: I work at Typemock]
You have three options - two of which requires some refactoring:

  1. Pass all arguments - like you already know this way you can pass mocks/stubs instead of "real" objects.
  2. Use IoC container - refactor your code to use the container to grab objects from your code and you can replace them with mocks/stubs.
  3. Use Typemock Isolator (.NET) that can fake future objects - objects that are instantiated inside your code from the test code. This option does not require refactoring and if you have a big code base it should be worth its code.

Design for testability is not always a good practice especially with existing project where you already written some code. So if you're starting clean or have a small project perhaps it's OK to pass objects as arguments to the class's constructor as long as you do not have too many parameters.
- If you do use IoC container.

If you do not want to change all of your existing code and/or do not want to design your code in a certain way to make it "more testable" (can cause some wierd looking code - use Isolator (or similar tool if you're using Java).

Dror Helper
+1  A: 

Only mock the objects that gets in the way when writing the unit tests. If a method creates an object to perform its tasks an you can probe its result, then there is no need to mock the class of the object that is creates.

Use mock when you want to isolate a class from other. Use mock so keep the tests away from

  • filesystems
  • databases
  • networks
  • object with unpredictable behavior (clock, random number generator ...)

Separate usage of object from their construction

philippe
A: 

I would try to use dependency injection on the class rather than have the class method create the object (as the selected answer recommends). When that doesn't make sense, consider making a factory class that produces the objects being created. You can then pass in that factory via dependency injection.

Frank Schwieterman
A: 

I prefer to mock just about everything around an object, and define the behavior of the object that is being tested in terms of calls to, and responses from, the associated objects.

To do this effectively requires that your interfaces be at the semantic level rather than the implementation level, in general.

kyoryu