Inversion of control

Question

I have been reading about inversion of control and dependency injection and I was wondering the following.

Is there a good way to tell when it is okay to initialise an object inside a method body of a class and when it's better to pass it as a constructor dependency?

For instance I was doing refactoring after finishing a TDD kata about generating greetings with some processing of some arguments. The code had some intermediate results that I used to construct the final string.

During refactoring I extracted some of the logic in their own classes by creating an instance at the point in the method where I needed them and passing the intermediate structures in their constructor and then call the method that operated on them.

Are there considerations in choosing to write a class that has one method that acts on its parameters vs writing a class that instead takes the same parameters in the constructor and the methods acts on its fields?

Here is what the code looked like after the first refactor

public class GreeterGenerator {
    private static final Greeter NO_NAME_GREETER = () -> "Hello, my friend."
    private final NameProcessor nameProcessor;

    public GreeterGenerator(NameProcessor nameProcessor) {
        this.nameProcessor = nameProcessor;
    }

    public String buildGreeting(String name) {
        return isEmpty(name)
                ? NO_NAME_GREETER.buildGreeting()
                : buildGreeting(new String[]{name});

    }

    private boolean isEmpty(String name) {
        return name == null || name.isEmpty();
    }

    public String buildGreeting(String[] names) {
        String[] processedNames = nameProcessor.splitAnyCommaSeparatedEntriesToSingleNames(names);

        var nameDifferentiator = new NameFilter(processedNames);

        var normalGreeter = new NormalGreeter(nameDifferentiator.filterNormalNames());
        var shoutingGreeter = new ShoutingGreeter(nameDifferentiator.filterShoutedNames());

        return new GreeterJoiner(normalGreeter, shoutingGreeter).joinGreetings();
    }
}

and this is after trying I moved the helper classes to constructor dependencies. The filtered names is just a wrapper around the two arrays that resulted from the methods in nameDifferentiator being called. The normalGreeter and shoutingGreeter were moved into the GreeterJoiner as constructor dependencies

public class GreeterGenerator {
    private static final String NO_NAME_GREETING = "Hello, my friend.";
    private final NameProcessor nameProcessor;
    private final NameFilter nameFilter;
    private final GreeterJoiner greeterJoiner;

    public GreeterGenerator(NameProcessor nameProcessor,
                            NameFilter nameFilter,
                            GreeterJoiner greeterJoiner) {
        this.nameProcessor = nameProcessor;
        this.nameFilter = nameFilter;
        this.greeterJoiner = greeterJoiner;
    }

    public String buildGreeting(String name) {
        return isEmpty(name)
                ? NO_NAME_GREETING
                : buildGreeting(new String[]{name});
    }

    private boolean isEmpty(String name) {
        return name == null || name.isEmpty();
    }

    public String buildGreeting(String[] names) {
        String[] processedNames = nameProcessor.splitAnyCommaSeparatedEntriesToSingleNames(names);

        var filteredNames = nameFilter.filterNames(processedNames);

        return greeterJoiner.joinGreetings(filteredNames);
    }
}

The code and commits (just filter out Greeter) can be seen here --> https://github.com/PhoenixRe32/tdd-exercises/tree/master/greeter It's a repo where I do my TDD katas as an exersice/refresher/or just pass the time (equivalent of doing sudoku I guess)

Answer 1

Are there considerations in choosing to write a class that has one method that acts on its parameters vs writing a class that instead takes the same parameters in the constructor and the methods acts on its fields?

There are two main reasons you'd abstract it away in a class:

• Reusability, i.e. you want to use this sublogic in other classes as well
• Cleanliness, i.e. the sublogic is sizeable and becomes a distraction to the parent class' other code.

Is there a good way to tell when it is okay to initialise an object inside a method body of a class and when it's better to pass it as a constructor dependency?

Use it as a constructor dependency when:

• There are multiple possible implementations for this abstraction
• You want the ability to switch implementation without changing the parent class, i.e. OCP-friendliness

Use direct initialization if:

• The subclass is considered a private implementation detail
• The subclass is a data container and has no meaningful behavior
• Consumers of the parent class have no vested interest in what the subobject is, that it is used, or how it is used.

This is a careful balance and not easy to enshrine into a universal mantra. A simple data struct can clearly be instantiated, anything you'd call a "service" should clearly be a constructor dependency. Inbetween those two, the lines get more blurry and people find arguments either way.

In general, I'd err towards constructor dependencies, unless you have a strong argument as to why it's not relevant.

a class that instead takes the same parameters in the constructor and the methods acts on its fields?

What's particular about your scenario is that you're effectively creating a helper method, which is generally a static method, but you've object for an object-oriented approach, and I suspect the object constructor works a lot like the static method body would.

This isn't wrong per se, but it's something I've heard senior developers disagree on.

• Some argue that static helpers "skip" the whole DI conversation and are considered as hardcoded data operations.
• Some argue that static helpers are dirty and should be converted to instanced services which do get injected. The main argument I've heard here is the core DI argument: avoid hardcoding at all costs.

I think there' a line to be drawn here in what constitutes a helper method that is worth converting to an instanced service. I wouldn't particularly ban all static logic, but I would try to inject things that are less than trivial.

var nameDifferentiator = new NameFilter(processedNames);

var normalGreeter = new NormalGreeter(nameDifferentiator.filterNormalNames());
var shoutingGreeter = new ShoutingGreeter(nameDifferentiator.filterShoutedNames());

return new GreeterJoiner(normalGreeter, shoutingGreeter).joinGreetings();

In your case, I'd even consider injecting a GreetingFactory which generates the needed Greeting objects based on the String[] processedNames.

Assuming your Greeter classes derive from a base class, I would've put the join logic in that base class, rather than in a separate GreeterJoiner class.

You didn't specify the contents, but something like:

Edit: I noticed too late that you're using Java and this example is C#, but I assume the intent of the example is readable.

public class Greeter
{
    public readonly IEnumerable<string> Names = new List<string>();

    public virtual void AddName(string name)
    {
        this.Names.Add(name);
    }

    public Greeter Join(Greeter otherGreeter)
    {
        var mergedGreeter = new Greeter();

        foreach(var name in this.Names)
            mergedGreeter.Add(name);

        foreach(var name in otherGreeter.Names)
            mergedGreeter.Add(name);

        return mergedGreeter;
    }
} 

public class WhisperingGreeter : Greeter
{
    public override void Add(string name)
    {
        this.Names.Add(name.ToLower());
    }
}

public class ShoutingGreeter : Greeter
{
    public override void Add(string name)
    {
        this.Names.Add(name.ToUpper());
    }
}

Answer 2

It depends on the type of object.

If you find yourself passing the same interface to each and every method of the object after you create it, you probably want to pass the interface to the constructor instead. There is obviously a strong relationship between the object and that interface, the object has no meaningful application without it.

If you have seven methods and only one of them depends on the interface, you want to pass it to that one method to keep your scope tight. The interface is not tied to the object, it is just tied to that one operation. So it has no place as a member of the object.