0

I need some design advice.

I have an application that will be reading data from various sources and doing checks and comparisons. My intention was to have all dependencies needed for data access (System.Data.SqlClient etc) contained within one set of Data Access classes (SqlImporter, JSONImporter etc.). Other classes that need data would then just use these classes to do the work. Perhaps they would pass a connection string or other information.

However, I needed to make unit tests for these Data Access classes. To make them testable, I made my Data Access classes rely on abstractions/interfaces and then pass in concrete implementations to allow me to pass in Mock objects from a unit test, similar to Mocking SqlConnection, SqlCommand and SqlReader in C# using MsTest.

The result is something like this:

using System.Data.SqlClient;

public class SqlImport {
   private IDbConnection conn;

   public SqlImport(IDbConnection conn){
      this.conn = conn;
   }

}

My problem is that, all classes that use these Data Access classes will now also need to rely on Data Access dependencies (System.Data.SqlClient). Does this somewhat defeat the purpose of having these classes? I mean, it is good for cohesion but...

I now have a class like this:

using System.Data.SqlClient;
using Importers;

public class Mapping {

   public Mapping (){
   }

   public void LoadMappingFromDatabase(string connString){
      SqlImport import = new SqlImport(new SqlConnection(connString));
      // Do Stuff including using import to query db
   }
}

Is this good design? Or am I better off just querying the DB inside LoadMappingFromDatabase() directly since the required dependencies are already in the class?

HelloWorld
  • 418
  • 3
  • 15

2 Answers2

1

You should just go a step further with Dependency Injection:

using System.Data.SqlClient;
using Importers;

public class Mapping {

   public Mapping (){
   }

   public void LoadMappingFromDatabase(SqlImport import){

      // Do Stuff including using import to query db
   }
}

Or event better if you inject the SqlImport through the constructor:

using System.Data.SqlClient;
using Importers;

public class Mapping {
   private SqlImport import;

   public Mapping (SqlImport import){
      this.import = import
   }

   public void LoadMappingFromDatabase(){

      // Do Stuff including using import to query db
   }
}
Michael Chekin
  • 568
  • 4
  • 11
  • 1
    My original design did include this dependency injection but I thought maybe it's better to define the Sql query used for loading the mappings inside the LoadMappingFromDatabase() method so the method has full control of the format of returned values... But then again, I could just add error handling/checks inside the method that verify the format of query defined by calling classes? maybe? What do you think is bbest – HelloWorld Jan 14 '21 at 17:03
  • As I understand, you want SqlImport wrap all the different connections to different data sources. You can still get those connections from import property inside LoadMappingFromDatabase and execute the specific queries using them. – Michael Chekin Jan 14 '21 at 17:26
0

Your design is fine. Having to have the using System.Data.SqlClient; header in your code in order to allow auto testability seems like a decent tradeoff. ;-)