6

In my workplace I am seeing an issue which I feel is wrong, but I am open to the idea that maybe the way I do it is wrong.

A minor example would be a class accepting configuration

//pull config from config file, then pass in to class
public class SomeClass()
{
   public SomeClass(string connectionString, string someOtherRequiredConfiguration)
   {
      //Do stuff
   }
}

In comparison, a class responsible for its own configuration

public class SomeClass()
{
    public SomeClass(){
       //Do work to pull from config file in constructor or in parameter
    }        
}

I think the second way makes DI much easier and also the class is responsible for retrieving the configuration it cares about.

Is there a prefered method?

Improve this question
8
  • 6
    I think the second way makes DI much easier are you sure? How you can inject different implementation of "pulling" configuration if class will do it by itself Commented Aug 13, 2018 at 10:02
  • 1
    You could have an IConfigRepository or something if you wanted to control that separately. If I was sure i wouldn't be asking. But pulling the config values and passing them in at the composition root feels wrong to me. Commented Aug 13, 2018 at 10:20
  • 1
    If you asking about should I inject an abstraction which "pull" required data or already "pulled" data, then I afraid there no clear answer with information you provided. If SomeClass can exist without configuration details, then passing IConfigurationLoader can be good enough idea. On other hand if SomeClass cannot be functional without configuration data, then, I think, having constructor with those details, will be more comprehensible approach. You can have some kind of builder for SomeClass then. Commented Aug 13, 2018 at 10:26
  • 6
    I strongly prefer passing a config to the constructor if the config is serialized (which it is if it is stored in a file). This goes well with the single responsibility prinicple: SomeClass already has one responsibility (otherwise you wouldnt need it), serializing it is another responsibility. Say you change the file format from xml to ini, then you only need to change SomeClassConfig. Depending on the serialization framework, you may not even have to change the config class. However, if SomeClass loads itself from some path, you have a hardcoded dependency to the file and its format. Commented Aug 13, 2018 at 10:35
  • 1
    I'm puzzled that no one else has pointed out that public class SomeClass(string connectionString, string someOtherRequiredConfiguration) is not valid C# syntax and that your two code examples are therefore very confused and confusing. Commented Aug 13, 2018 at 17:22

2 Answers 2

Reset to default
9

Both is not really great.

The first version is slightly better, because it at least attempts to be able to inject something.

The problem is, strings cannot be injected based on the name of the parameter (at least none of the standard frameworks does this and although theoretically possible, nobody in their right mind would try that).

You need a type to properly inject something. 

public interface ISomeClassOptions
{
    public string ConnectionString { get; }

    public string SomeOtherRequiredConfiguration { get; }
}

public class SomeClass(ISomeClassOptions options)
{
   //Do stuff
}

This class can now be used without much manual configuration with a dependency injection container. You only need to find a way to fill your options from somewhere.

However, the use of a connection string and other configuration leads me to a guess that only you can verify: The connection string should not be an option. Instead the SomeClass should probably get a IDbConnection or DbContext or other class injected that handles the nitty gritty details of what the database is. Because it might be a SqlConnection for production or maybe just an in memory database connection for testing. That is up to the container to decide and should not be inside your SomeClass logic.

If you are using .NET Core, have a look at the new Options pattern.

Improve this answer
6
  • While I get your point, I'm not quite sure you should be equating dependency injection and method parameters. Method parameters may be a "dependency" of the method (in the sense that the outcome can be different based on the parameter value), but that's not really an injected dependency. Dependency injection focuses on how classes are stitched together, not which particular values are sent to a given object. If anything, dependency injection seeks to ensure that the outcome of a method doesn't change when you pass a different parameter value, i.e. it works the same for any IFoo. Commented Aug 13, 2018 at 13:31
  • 3
    @Flater, I disagree. "Dependency injection" is simply "tell, don't ask", ie the thing, whether that be a function, object or entire library is told what its dependencies are, rather than it calling at to some service to find them. So parameters to a function most definitely are an example of dependency injection. Commented Aug 13, 2018 at 17:20
  • @Flater: though you are literally correct, for the correctness of this answer it does not matter if the first approach can be called DI or not. Commented Aug 13, 2018 at 21:18
  • I think the Options Patterns is what is needed here. Some discussion points too. Commented Aug 14, 2018 at 8:34
  • The given reason is "strings are difficult to inject", but in the proposed solution, you still need to do that - just to another class. Anything you would do to get those strings into your implementation of ISomeClassOptions - e.g. factory, hardcode it, config file - you could just as well do for SomeClass. Regarding injection of strings, you're back in the very same situation, just with an extra class. I do agree with using an own type, but the reasoning here doesn't make sense to me - maybe I'm just missing something? Commented Aug 15, 2018 at 10:05
4 
//pull config from config file, then pass in to class
public class SomeClass(string connectionString, string 
someOtherRequiredConfiguration)
{
   //Do stuff
}

The rest of your answer talks about DI, but this particular snippet isn't really DI-oriented. This is a normal run-of-the-mill parametrized constructor.

I do wonder about someOtherRequiredConfiguration. Based on the name, I'm going to assume you've oversimplified the example and that the real code contains meaningful parameters.

public class SomeClass()
{
    //Do work to pull from config file in constructor or in parameter
}

the class is responsible for retrieving the configuration it cares about

Objectively correct, but that doesn't answer whether it's a good idea or not. I will elaborate on this point.

I think the second way makes DI much easier

Quite the opposite. What you've done here is removed the option to configure your class the way you want to. This makes it harder to mock, not easier. You're forcing yourself to create a new (probably derived) class whenever to you want to test against a different external resource (= different connectionstring).

You could have an IConfigRepository or something if you wanted to control that separately.

What you say isn't wrong, but I just want to reframe what you said: your proposed solution now requires you to create an IConfigRepository before you can properly test. It's not impossible to do so, but it creates more work to have testable code, which is the opposite of what DI tries to achieve (increasing testability)

But pulling the config values and passing them in at the composition root feels wrong to me.

The fact that you refer to SomeClass as a composition root makes me suspect that you're thinking about DI/testability in an unusual way.

The value of a parameter (connectionstring) has little to do with dependency injection. Dependency injection focuses on stitching together custom classes in a way that you can easily separate them in the future (e.g. testing a single component, or swapping a particular component for another one).

Should a class be responsible for its own configuration?

It highly depends on the configuration you're focusing on. Some things don't belong in a config file to begin with and can simply be set as a const value.

If you can guarantee that SomeClass will always use a particular config key, without fail, then you can retrieve the key from inside the class (constructor or otherwise).

If SomeClass needs to be able to toggle between several connectionstrings (e.g. an application that uses multiple databases at the same time), then it needs to be configurable. A constructor parameter is the better approach here.

Improve this answer
3
  • With regards to composition root, I mean the SomeClass would be created at the apps composition root (using manual DI, not a container), so currently there code calling the config file before creating the concrete class, so it can be passed it. I think the Options Pattern mentioned above might be the best solution Commented Aug 13, 2018 at 14:56
  • @James: That's valid technical approach, but it's not really DI specifically. Just because DI often occurs via constructor parameters doesn't mean that all constructor parameters are being used for DI. This is a fairly normal configuration that is unconnected to DI itself (even though there are some similarities, I grant you). Commented Aug 13, 2018 at 14:58
  • Upvoted this answer too as it makes good points, but I think Options Pattern is the answer I need in this case. Commented Aug 14, 2018 at 8:35

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.