CRUD commands to SQL database

Question

I have a large application that focuses on using dependency injection. I've made a small (as I possibly could) piece of sample code to make this question more manageable. Essentially, I have a lot of boiler-plate code occurring for a number of commands that call stored procedures and return a response object back to the caller. I'd really like to find a more generic (if possible) way of doing this.

Normally, all of this code sits inside a Web Api and would have controllers executing the commands.

The complete code example is as follows (NOTE The code to refactor is at the very bottom, the rest is just supporting code):

Request/Response objects

Request

All requests inherit from BaseRequest which just contains the identifier for the api performing the request (this is verified within the proc):

 public class BaseRequest
 {
 public string Identifier { get; set; }
 }

Here is an example of a request class for a command:

public class ReadAssetRequest : BaseRequest
 {
 public int TypeId { get; set; }
 public int OwnershipId { get; set; }
 public int GroupId { get; set; }
 public IEnumerable<int> StatusIds { get; set; } 
 }

Response

All responses inherit from BaseResponse which just contains a list of errors from the stored procedures (if any):

public class BaseResponse
 {
 public List<int> Errors { get; set; }
 }

Here is an example of a response class for a command:

public class ReadAssetResponse : BaseResponse
 {
 public AssetInformation AssetInformation { get; set; }
 }

This is the class for the object being returned:

public class AssetInformation
 {
 public int Id { get; set; }
 public string Uprn { get; set; }
 public string Address { get; set; }
 public int? OSLocation { get; set; }
 }

To talk to the database. There is a database helper:

DatabaseHelper Interface

public interface IDatabaseHelper
 {
 void ExecuteNonQuery(DatabaseCommandInfo data);
 DataSet GetDataSet(DatabaseCommandInfo data);
 DataTable GetDataTable(DatabaseCommandInfo data);
 }

DatabaseHelper Class

public class DatabaseHelper : IDatabaseHelper
 {
 private readonly string connectionString;
 public DatabaseHelper(string connectionString)
 {
 this.connectionString = connectionString;
 }
 public DataSet GetDataSet(DatabaseCommandInfo data)
 {
 var ds = new DataSet();
 using (var con = new SqlConnection(connectionString))
 {
 con.Open();
 using (var cmd = GetSqlCommand(data, con))
 {
 using (var rdr = cmd.ExecuteReader())
 {
 ds.Load(rdr, data.Option, data.TableNames);
 }
 cmd.Parameters.Clear();
 }
 }
 return ds;
 }
 public DataTable GetDataTable(DatabaseCommandInfo data)
 {
 var dt = new DataTable();
 using (var con = new SqlConnection(connectionString))
 {
 con.Open();
 using (var cmd = GetSqlCommand(data, con))
 {
 using (var rdr = cmd.ExecuteReader())
 {
 dt.Load(rdr);
 }
 cmd.Parameters.Clear();
 }
 }
 return dt;
 }
 public void ExecuteNonQuery(DatabaseCommandInfo data)
 {
 using (var con = new SqlConnection(connectionString))
 {
 con.Open();
 using (var cmd = new SqlCommand(data.StoredProcName, con))
 {
 cmd.CommandType = data.CommandType;
 cmd.Parameters.AddRange(data.Parameters);
 cmd.ExecuteNonQuery();
 cmd.Parameters.Clear();
 }
 }
 }
 private SqlCommand GetSqlCommand(DatabaseCommandInfo data, SqlConnection sqlConnection)
 {
 var cmd = new SqlCommand(data.StoredProcName, sqlConnection)
 {
 CommandType = data.CommandType
 };
 if(data.Parameters != null)
 cmd.Parameters.AddRange(data.Parameters);
 return cmd;
 }
 }

The database helper takes a DatabaseCommandInfo object so it knows what stored proc to call and with what SqlParameters:

DatabaseCommandInfo class

 public class DatabaseCommandInfo
 {
 public string StoredProcName { get; private set; }
 public SqlParameter[] Parameters { get; private set; }
 public string[] TableNames { get; private set; }
 public LoadOption Option { get; private set; }
 public CommandType CommandType { get; set; }
 public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams)
 {
 StoredProcName = storeProcName;
 Parameters = spParams;
 CommandType = CommandType.StoredProcedure;
 }
 public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams, string[] tableNames)
 {
 StoredProcName = storeProcName;
 Parameters = spParams;
 TableNames = tableNames;
 Option = LoadOption.OverwriteChanges;
 CommandType = CommandType.StoredProcedure;
 }
 }

Helper/Extension Methods

The command uses some Helper/Extension methods.

DataRowExtensions

Extension methods used to help retrieval of values from a DataRow:

public static class DataRowExtension
 {
 public static T GetValue<T>(this DataRow row, string columnName)
 {
 if (row != null && row.Table.Columns.Count > 0 && row[columnName] != DBNull.Value)
 {
 return (T)Convert.ChangeType(row[columnName], typeof(T));
 }
 return default(T);
 }
 public static T? GetNullableValue<T>(this DataRow row, string columnName) where T : struct
 {
 if (DBNull.Value.Equals(row[columnName]))
 {
 return null;
 }
 return (T)Convert.ChangeType(row[columnName], typeof(T));
 }
 }

Helper method to serialize object as XML

Used when a SQL parameter is not a primitive type, the value is passed as XML to the proc.

public static class ListExtensions
 {
 public static string IdsToXml(this IEnumerable<int> ids)
 {
 var idList = ids.ToList();
 if (!idList.Any())
 return new XElement("Ids").ToString();
 var xmlElements = new XElement("Ids", idList.Select(i => new XElement("x", new XAttribute("i", i))));
 return xmlElements.ToString();
 }
 public static string ToXml<T>(this T items)
 {
 return Serializer.SerializeObject(items);
 }
 }

Serializer class

public static class Serializer
 {
 public static string SerializeObject<T>(T toSerialize)
 {
 var xmlSerializer = new XmlSerializer(toSerialize.GetType());
 using (var textWriter = new StringWriter())
 {
 xmlSerializer.Serialize(textWriter, toSerialize);
 return textWriter.ToString();
 }
 }
 }

Actual command to refactor

With all the above supporting code. The following is an example of a simple command that contains boiler-plate code I'd like to refactor. The flow of the commands are:

1. Create an empty instance of the response object
2. Create the SqlParameters (Note the SQL parameter names are always the same as a request object property names. The data types are also identical for primitive types, anything else is serialized and passed as an XML parameter).
3. Create the DatabaseCommandInfo
4. Call a DatabaseHelper method and return a result (could be scalar object, DataSet/DataTable)
5. Populate the response object with the result from the database helper.
6. If a SqlException is thrown, store the error code and return the response with that code.

public class ReadAsset
 {
 private const string StoredProc = "up_Assets_ReadAsset";
 private readonly IDatabaseHelper databaseHelper;
 public ReadAsset()
 {
 databaseHelper = new DatabaseHelper("Data Source=.; Initial Catalog=Assets; integrated security=true;");
 }
 /// <summary>
 /// Constructor used to inject dependencies
 /// </summary>
 /// <param name="databaseHelper"></param>
 public ReadAsset(IDatabaseHelper databaseHelper)
 {
 this.databaseHelper = databaseHelper;
 }
 public ReadAssetResponse Execute(ReadAssetRequest request)
 {
 var response = new ReadAssetResponse();
 var sqlParams = new[]
 {
 new SqlParameter("@TypeId", request.TypeId),
 new SqlParameter("@OwnershipId", request.OwnershipId),
 new SqlParameter("@GroupId", request.GroupId),
 new SqlParameter("@StatusIds", request.StatusIds.ToXml()),
 };
 var dbCommandInfo = new DatabaseCommandInfo(StoredProc, sqlParams, new[] {"AssetInfo"});
 try
 {
 var dataTable = databaseHelper.GetDataTable(dbCommandInfo);
 response.AssetInformation = new AssetInformation();
 if (DataTableIsNotPopulated(dataTable))
 return response;
 var row = dataTable.Rows[0];
 response.AssetInformation.Id = row.GetValue<int>("Id");
 response.AssetInformation.Address = row.GetValue<string>("Address");
 response.AssetInformation.Uprn = row.GetValue<string>("Uprn");
 response.AssetInformation.OSLocation = row.GetNullableValue<int>("OSLocation");
 }
 catch (SqlException sqlException)
 {
 response.Errors = new List<int> {sqlException.ErrorCode};
 }
 return response;
 }
 private static bool DataTableIsNotPopulated(DataTable dataTable)
 {
 return dataTable == null || dataTable.Rows == null || dataTable.Rows.Count != 1;
 }
 }

I haven't included the database information (i.e. table/procs, etc) as it is not relevant or required here.

• BCdotWEB: Why the calls to cmd.Parameters.Clear();? cmd is inside a using block and you're not re-using it, so what's the point?
• JBond: Very good point! No idea why it's there. I can only imagine that prior versions of the code weren't inside a using block and during the refactor they weren't removed.

Answer 1

Don't do this:

 public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams)
 {
 StoredProcName = storeProcName;
 Parameters = spParams;
 CommandType = CommandType.StoredProcedure;
 }
 public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams, string[] tableNames)
 {
 StoredProcName = storeProcName;
 Parameters = spParams;
 TableNames = tableNames;
 Option = LoadOption.OverwriteChanges;
 CommandType = CommandType.StoredProcedure;
 }

Instead, use constructor chaining:

public DatabaseCommandInfo(string storeProcName, SqlParameter[] spParams)
 : this(storeProcName, spParams, new string[])
{
}

The same is true for ReadAsset:

public ReadAsset()
 : this(new DatabaseHelper("Data Source=.; Initial Catalog=Assets; integrated security=true;"))
{
}

---

The code isn't consistent: row[columnName] != DBNull.Value vs DBNull.Value.Equals(row[columnName]).

---

Why is this checked: row.Table.Columns.Count > 0 ?

---

In both GetValue and GetNullableValue you repeatedly call row[columnName]. Call it once and store the value in a variable and work with that variable.

---

In IdsToXml the element name "Ids" is used twice, so ideally it should be a const.

---

Your list of SqlParameter is missing the SqlDbType. I'd prefer this:

 var sqlParams = new[]
 {
 new SqlParameter("@TypeId", SqlDbType.Int).Value = request.TypeId,

---

Now, looking at your "problem": it is inevitable that there has to be some place where you need to actually do something specific. In this case there's ReadAsset where you get the ReadAssetRequest, convert that to sqlParams and dbCommandInfo, use these to try to get a dataTable from the databaseHelper, convert that to a ReadAssetResponse.

I'm sure some of that can be moved to a Helper class or a base class, and if you're really hardcore you can:

• build a mapper that can take any Request class and convert it to a list of SqlParameters etc. via reflection.
• Perhaps the names of the SqlParameters are the same as the name of the properties, and if they aren't there's an Attribute on top of the property.
• The name of the stored proc can be in a dictionary somewhere, with the type of the Request as the key.
• And another mapper can convert the datatable to the Response object.
• Even that DataTableIsNotPopulated check could be configured somewhere (because now you require a single result, but another response might require multiple results), etc.

Which means you perhaps wouldn't need a ReadAsset class anymore, since all of the actions inside it are actually a bunch of configurations used by helper classes. And thus instead of adding a single Asset class you now need to remember to add various configurations at various places.

Is that an improvement? I've worked with code like that, and while I did admire it, at times I did feel like Alice falling down the rabbit hole, ending up with dozens of code pages open in Visual Studio trying to figure out what needed to be altered where in order to get all of the components to play nice. And three months later I again needed to re-acquaint myself with the system before I could add another configuration.

Now, I can't say I'm a big fan of the code you show us here. The usage of arrays instead of IEnumerable<>, the apparent mixing of UI (request, response) and db (SqlParameter, DataTable),... I'd expect the db-related code to be in a separate layer and that you'd work with business entities. Why not use Entity Framework instead of mapping datatables to your custom classes? And do you really need to catch SqlExceptions?

Answer 2

response.AssetInformation = new AssetInformation();
if (DataTableIsNotPopulated(dataTable))
 return response;
var row = dataTable.Rows[0];
response.AssetInformation.Id = row.GetValue<int>("Id");
response.AssetInformation.Address = row.GetValue<string>("Address");
response.AssetInformation.Uprn = row.GetValue<string>("Uprn");
response.AssetInformation.OSLocation = row.GetNullableValue<int>("OSLocation");

Initializing your response.AssetInformation using the DataRow isn't the responsability of the Execute method. Your AssetInformation class should have a Create method or a constructor that takes as a parameter a DataRow. Because if you need to change the DataRow's logic one day, you'll need to change it in one place instead of all of the places where you initialize it.

Something like :

public class AssetInformation
{
 public int Id { get; set; }
 public string Uprn { get; set; }
 public string Address { get; set; }
 public int? OSLocation { get; set; }
 public AssetInformation(){}
 public AssetInformation(DataRow dataRow)
 {
 Id = dataRow.GetValue<int>("Id");
 Address = dataRow.GetValue<string>("Address");
 Uprn = dataRow.GetValue<string>("Uprn");
 OSLocation = dataRow.GetNullableValue<int>("OSLocation");
 }
}

Then you could do :

try
{
 var dataTable = databaseHelper.GetDataTable(dbCommandInfo);
 response.AssetInformation = DataTableIsNotPopulated(dataTable) ? new AssetInformation() : new AssetInformation(dataTable.Rows[0]);
}
catch (SqlException sqlException)
{
 response.Errors = new List<int> {sqlException.ErrorCode};
}
return response;

Much shorter isn't it?

Your indentation is flawed, I don't know if it's related to pasting it here or if it's this way in the IDE. Try to respest the IDE's indentation, why is more spaced. Space is cheap, use it ;)

Your Connection String shouldn't be hardcoded in the application, get it from a configuration file (web.config or app.config) or at least receive it as a parameter somewhere.