Am I breaking the DI pattern rules

Question

I'm very new to DI so my knowledge isn't the greatest. I am currently working on a Asp.Net Core MVC application.

Suppose I have a DLL which returns a DataSet (I have full control over this DLL and can change it as required).

So the code in the DLL looks like:

public class DataStore : IDataStore
{       
   private readonly IConfiguration _configuration;       

   public DataStore(IConfiguration configuration)
   {
       _configuration = configuration;
   }

   public DataSet GetDataBySP(string spName, SqlParameter[] parameters = null)
   {
      DataSet ds = new DataSet();
      using(var con = new SqlConnection(_configuration.GetConnectionString("conStr")))
      {
         using(var cmd = new SqlCommand(spName, con))
         {
              cmd.CommandType = CommandType.StoredProcedure;

              if (parameters?.Length > 0)
                 cmd.Parameters.AddRange(parameters);

              con.Open();

              using (var da = new SqlDataAdapter(cmd))
              {
                 da.Fill(ds);
              }
         }
      }
      return ds;
   }
}

Now assuming I have a class named Foo which only contains two properties as follows:

public interface IFoo
{
   Foo GetFooData();
}

public class Foo : IFoo
{
    private readonly IDataStore _dataStore;

    public int CurrentCount { get; set; }
    public int MaxCount { get; set; }

    public Foo(DataRow dr)
    {
        CurrentCount = dr.Field<int>("CurrentCount");
        MaxCount = dr.Field<int>("MaxCount");
    }

    public Foo(IDataStore dataStore)
    {
        _dataStore = dataStore;
    }
}

In Foo I have a method called GetFooData as follows:

public Foo GetFooData()
{
    var ds = _dataStore.GetDataBySP("sp name");

    //is the below code breaking the DI pattern rule?
    return ds.Tables[0].AsEnumberable().Select(f => new Foo(f)).FirstOrDefault();
}

In the Startup.cs inside ConfigureServices(IServiceCollection services) method I do the following:

services.AddScoped<IFoo, Foo>();

So my question is, by doing new Foo(f) inside the Select() am I breaking the DI pattern? If yes, can you advice on how to overcome this please.

Answer 1

I would have solved your problem through usage of repositories and data transfer objects (DTO), like this:

Some public interfaces which you would be able to inject into your code:

    public interface IFooRepository
    {
        FooDto GetFirst();
    }

    public interface IDataSetFactory
    {
        DataSet GetDataSet(string spName, SqlParameter[] = null);
    }

Some DTO to pass data from one class to another (and only data):

    public class FooDto
    {
        public int CurrentCount { get; set; }
        public int MaxCount { get; set; }
    }

Some internal implementation, which hidden from eye of interface user:

    internal sealed class FooRepository : IFooRepository
    {
        private readonly IDataSetFactory _dsFactory;
        public FooRepository(IDataSetFactory dsFactory)
        {
            _dsFactory = dsFactory;
        }

        public FooDto GetFirst()
        {
            return _dsFactory.GetDataSet("sp name")
                             .Tables[0]
                             .AsEnumberable()
                             .Select(Map)
                             .FirstOrDefault();
        }
        private FooDto Map(DataRow dr)
        {
            return new FooDto
                   {
                       CurrentCount = dr.Field<int>("CurrentCount"),
                       MaxCount = dr.Field<int>("MaxCount")
                   };
        }
    }

    internal sealed class DataSetFactory : IDataSetFactory
    {
        public DataSetFactory() { }
        private DataSet GetDataSet(string spName, SqlParameter[] = null)
        {
            //set up sql parameters, open connection
            return ds;
        }
    }

And given some specific DI framework (Ninject, DryIoc, etc), you can inject those interfaces to any place in the code. The great benefit of using DI - it forces you to design your application correctly (interfaces separated from implementation), so at some point in time you can implement your IFooRepository to read data from stars in the sky, or from file, or to return empty results in testing, or whatever you desire it to do.

If you design incorrectly - it probably will not compile, or will be overcomplexed. Good usage of DI is hard question, but estimate is that 99% of your *.cs files will NOT reference ANY DI framework references:

using System.IO;
using Ninject; //<-- I mean, this one. If it is everywhere, something is overcomplexed and you are not using framework correctly.
using System.Media;

Answer 2

Yes, you're violating the spirit of DI, but maybe not for the reason you think.

As one of the comments points out, you are potentially violating SRP with Foo. You say at one point that it "only contains the two properties", but it also contains logic and takes & stores a DataStore reference. If the two properties truly were all that Foo contained, it would essentially be a DTO ... a dumb bag of properties with no behavior or business rules, and in that case, I would say you were not violating the spirit of DI simply by calling, essentially new Foo(properties) ... because an anonymous object or a Tuple could serve the same purpose ... it would essentially reduce Foo to being content. DI is not about "never calling new" ... it's about logical code not resolving its own dependencies to execute its logic.

Since, in your example, you have also given Foo the responsibility of interrogating the DataSource to find the records that have these properties, you do have that data retrieval logic inside of Foo, and you're definitely getting into some gray area. As mentioned, your class now has two different and non-intersecting responsibilities. You're starting to tread close to DDD / rich domain concepts, where Foo would be responsible for transporting its own data and handling business concerns about manipulating that data. But DDD is not the most DI-friendly of architectures and is typically considered better suited for large & complex domains than for small ones.

The whole idea with DI is that units of code (classes) should not resolve their own dependencies, in order to help manage/organize/reduce complexity. If you factor out the GetFooData() method on Foo to some other class which is supplied by DI, and reduce Foo to a simple DTO/property bag, then I think you would at least have an argument that you are not violating the spirit of DI, because then Foo is not a behavioral dependency of the code that converts a DataRow to a Foo.