CRUD commands to SQL database

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 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 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 GetData

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?

StackExchange Code Review Q#106233, answer score: 6