# Thursday, January 26, 2012
« I am done with DasBlog | Main | Why I think Concrete5 has a great future... »
These past few weeks I've been occupied with replacing the security infrastructure of our flagship application to make it more flexible (more on that in another post). As part of the rework, I took the opportunity to cleanup a lot of code. Well actually, the new security API that I designed made it pretty much impossible for a developer to willy-nilly abuse the API. This meant that to make the app work with the new API, the app had to be cleaned up and simplified in many places.

A large part of this cleanup involved replacing lots of copy-pasted, repeated code (the polar opposite of DRY) with a single implementation (a "single source of truth" as we like to call these things at IU). This process pitted me squarely against the data access layer, which exposed an API that looked like this:
class DataAccess
  public enum Connection { Db1, Db2 }

  public DataSet GetDataSet(string procName, ArrayList procParams, Connection connection)
    var connectionString = ConfigurationManager.ConnectionStrings[...] ;
    using (SqlConnection conn = new SqlConnection(...))
      // etc.

And here's how the client code was using it:

class MyDao
  private const ProcName = "GetStuff" ;
  // Assume the stored procedure did something like this:
  // @"select milk from dbo.cow where sound = @sound" ;

  private IEnumerable<MyDTO> GetStuff()
    // Add parameters
    ArrayList pars = new ArrayList() ;
    pars.Add(new SqlParameter("@sound", moo)) ;
    // Use a (eek) singleton instance
    DataSet ds = DataAccess.Instance.GetDataSet(ProcName, pars, DataAccess.Connection.Db1) ;

    // Just assume nothing went wrong, of course.
    if (ds.Tables[0].Rows.Count > 0)
      // Deserialize objects from DataRows and return
There are several obvious problems with this API:
It forces the client code to use a stored procedure.
There was no equivalent that assumed CommandType.Text. Other methods in the library did things like get DataTables instead of DataSets and they assumed CommandType.Text. So you could either run a stored procedure and get a DataSet or run a SQL command and get a DataTable. Why was it done this way? Because the developer added exactly the methods needed to get the job done, added them as the need arose and never had the opportunity to refactor.
It forced the client to make decisions about the connection to use on every single request.
Imagine if a table or source moved from one database to another. This actually happens once in a while in our environment as we try to use more and more of the university central data warehouse (Oracle) as opposed to our internal databases (SQL Server). Imagine what you would need to do if you actually had to change your queries to run against Oracle or if you simply moved queries from Db1 to Db2. You would have to do a massive search-and-replace, which would almost certainly introduce a few bugs of its own.
It is way too verbose and repetitive
Imagine writing code like the client code listing above hundreds of times. The part where an ArrayList is created and populated is boilerplate nonsense that adds no value to the business. Yet I actually found those lines actually repeated hundreds of times in the code base. I am not really sure why the API didn't at least use the params keyword.

Anyway, thanks to other pressing deadlines and the fact that despite the code issues, the application worked very well, this code lingered on in the code base for months as features came and went and the code around it was committed and updated.

Until now, where I finally replaced the data access methods with a unified signature that looks like this:

public virtual DataSet GetDataSet(string selectCommandText, object parameters = null, CommandType commandType = CommandType.Text)
The signature makes it clear what is assumed. There is no connection or connection string being used because the client code uses a data access object that is already configured with the appropriate connection string required (via Dependency Injection using Ninject). And the best part? The parameters is expected to be an anonymous type that is unpacked and converted into a set of DbParameter (by iterating over the properties using TypeDescriptor)  Yes, the code does not even assume that you are running against SQL Server, because the constructor of this object looks like this:

protected AbstractDataAccess(AbstractDbFactory dbFactory)
And what does AbstractDbFactory do? It provides a provider-specific implementations of DbCommand, DbConnection, DbParameter and so on. It even inserts the appropriate @ or : in front of the parameter name depending on the database.

As for the configuration itself, this is how I do it with Ninject:
  .WithConstructorArgument("connectionString", Kernel.Get<ConfigurationItems>().Db1ConnectionString);

  .WithConstructorArgument("dbFactory", Kernel.Get<AbstractDbFactory>("Db1"));
Put together, client code now looks like this:

class MyDao

  public MyDao(AbstractDataAccess dataAccess)
    // Specify with Ninject which pre-configured
    // AbstractDataAccess object to resolve this
    // this dependency to using constructor
    // injection.

  public IEnumerable<MyDTO> GetStuff()
    var ds = _DataAccess.GetDataSet(procName, new { sound = "moo" }, CommandType.StoredProcedure) ;
    // Deserialize and return
// etc
And in cases where there are no parameters and the client just wants to execute a select query, it becomes:

Sweet :)

Thursday, January 26, 2012 3:34:43 AM (GMT Standard Time, UTC+00:00)  #     |  Comments [0]  |  Trackback