CODEWITHSUNDEEP
javac#dependency-injection
Sergey
Sergey

Reputation: 189

Constructor injection, avoid non-dependency parameters

I need to refactor existing abstract class to implement Dependency Injection, but this class has two constructors that takes other parameters.

public abstract class MyClassBase
{
    MyClassBase(int settingId)
    {
        _settingId = settingId;
    }

    MyClassBase(Setting setting)
    {
        _setting = setting;
    }
    ...
}

I need to inject some interfaces and avoid passing any other parameter like "settingId" and "Setting" in constructor. So my idea is to create two methods to setup these parameters once we create the instance of this class.

public abstract class MyClassBase
{
    private readonly IOneService _oneService;
    private readonly ITwoService _twoService;

    MyClassBase(IOneService oneService, ITwoService twoService)
    {
        _oneService = oneService;
        _twoService = twoService;
    }

    protected void SetupSetting(int settingId)
    {
        _settingId = settingId;
    }

    protected void SetupSetting(Setting setting)
    {
        _setting = setting;
    }

    protected Setting Setting
    {
        get 
        {
            if(_setting == null)
            {
                _setting = _oneService.getSettingById(_settingId);
            }

            return _setting;
        }
    }  
}

But it does not look as a proper solution, because if developer forget to run one of these methods just after instance creation we can get an exception(object not set to reference...) in future. How should I do it properly?

Upvotes: 2

Views: 652

Answers (2)

Steven
Steven

Reputation: 172865

Your injectables should have just one constructor. Having multiple constructors is an anti-pattern. Everything that the class requires to run must be passed in through the constructor; constructing the object using multiple steps leads to temporal coupling, which is a design smell.

So in general, I agree with @realnero's solution, although abstracting the Settings object with an interface might not be useful. Interfaces are meant to abstract behaviour, but such settings object will typically only contain data, no behaviour. Adding an abstraction will therefore not work.

Although injecting a Settings object can be fine, such object is often misused. You should not pass in more into the constructor than such class directly needs itself. If the Settings object contains values for the whole application, it will become unclear what values such class needs. It will also cause you to change the Settings object every time the configuration changes and there will be many consumers depending on this ever growing Settings class.

These settings classes are often used to dispatch to the configuration file. In other words, when a consumer asks for a value from the Settings class, it queries the configuration file. This results in a configuration file that is read lazily. This makes it really easy for an application to fail at runtime, because of typos in the configuration file. Instead you rather want the application to fail directly at start up (fail fast).

So instead, you should just inject the configuration values that such component needs and those values should be read from the configuration file at application start up. This allows the application to fail fast when the config is incorrect, makes it very clear what the component requires and prevents you from having central configuration classes where the whole application depends upon.

But having base classes with dependencies is a code smell by itself. Without a concrete example it's impossible to say what how to refactor, but I think there are two design mistakes that cause the use of base classes.

Developers often move cross-cutting concerns into base classes. This will cause those base classes to grow and become hard to maintain, and it causes the complexity of the base class to be intertwined with the derived class, making them complex and hard to test as well. Instead, cross-cutting concerns should be added using decoration or interception. The decorator design pattern is very effective in applying cross-cutting concerns. It allows the services to have no base class at all and provides great flexibility when it comes to adding new cross-cutting concerns. Take a look at this article for instance to get some ideas about this.

In case the base class doesn't handle cross-cutting concerns, but central application logic that many derived classes reuse, abstracting this base class logic into a separate service is often a much better approach. When doing this, this new service can be injected into your components, which allows them to make them oblivious about the dependencies this new service itself has. It prevents you from having to pass in dependencies of the base class and calling the base constructor. One of the ways to do this is using Aggregate Services.

Upvotes: 4

realnero
realnero

Reputation: 994

No. You should get all needed data from your dependent interface. Like:

MyClassBase(IOneService oneService, ITwoService twoService, ISettings set)
{
    _setting = set;
    ....
}

Upvotes: 3

Related Questions