Method that fetches rows from database and maps it to a Model class with SqlDataReader

Question

So, I have a project that has a lot of methods that look alike. In the below method as you can see it just fetches a bunch of rows from database and maps it to a model class.

In this case ParkingSlipDetails class. There are several such methods in the project that do the exact same thing only with a different set of data and a different model class. There is no complicated business logic here. Just pure fetching data and mapping it to a model.

Something like this:

  public CustomReturn < List < ParkingSlipDetails >> GetAllParkingSlips(int offset, int fetch)
    {
        SqlConnection sqlConnection = null;
        try
        {
            List < ParkingSlipDetails > ParkingSlipDetailsList = new List < ParkingSlipDetails > ();
            sqlConnection = (SqlConnection) sqlHelper.CreateConnection(); //sqlHelper is a nugget and not in my control
            sqlConnection.Open();
            SqlCommand cmd = new SqlCommand(Constants.GetAllParkingSlips, sqlConnection);
            cmd.CommandType = System.Data.CommandType.StoredProcedure;
            cmd.Parameters.Add("@CreatedBy", SqlDbType.NVarChar).Value = gepservice.GetUserContext().UserId;
            cmd.Parameters.Add("@OffsetRows", SqlDbType.NVarChar).Value = offset;
            cmd.Parameters.Add("@FetchRows", SqlDbType.NVarChar).Value = fetch;
            var refCountdr = (RefCountingDataReader) sqlHelper.ExecuteReader(cmd);
            var sqlDr = (SqlDataReader) refCountdr.InnerReader;
            if (sqlDr != null)
            {
                while (sqlDr.Read())
                {
                    ParkingSlipDetails ParkingSlipDetails = new ParkingSlipDetails();
                    ParkingSlipDetails.Id = Convert.ToInt32(sqlDr[FileUploadConstants.Id]);
                    ParkingSlipDetails.FileName = Convert.ToString(sqlDr[FileUploadConstants.FileName]);
                    ParkingSlipDetails.FileUri = Convert.ToString(sqlDr[FileUploadConstants.FileURI]);
                    ParkingSlipDetails.PONumber = Convert.ToString(sqlDr[FileUploadConstants.DocumentNumber]);
                    ParkingSlipDetails.UploadDate = Convert.IsDBNull(sqlDr[FileUploadConstants.UploadDate]) ? DateTime.UtcNow: Convert.ToDateTime(sqlDr[FileUploadConstants.UploadDate]);
                    ParkingSlipDetails.FileStatus = (Status)(Convert.ToInt16(sqlDr[FileUploadConstants.FileStatus]));
                    ParkingSlipDetails.ReceiptNumber = Convert.ToString(sqlDr[FileUploadConstants.RecieptNumber].ToString());
                    ParkingSlipDetails.TotalRows = Convert.ToInt32(sqlDr[FileUploadConstants.TotalRows]);
                    ParkingSlipDetails.CreatedBy = gepservice.GetUserContext().UserId;
                    ParkingSlipDetailsList.Add(ParkingSlipDetails);
                }
            }
            return new CustomReturn < List < ParkingSlipDetails >> (ParkingSlipDetailsList);
        }
        catch(Exception ex)
        {
            LogError("ManageParkingSlipDataAccess", "GetAllParkingSlips", "Error", ex);
            return null
        }
        finally
        {
            if (sqlConnection != null)
            sqlConnection.Close();
        }
    }

I think there is a lot of scope for refactoring here since there is a lot of places where I would repeat :

sqlConnection = (SqlConnection)sqlHelper.CreateConnection(); 
sqlConnection.Open();

and

var refCountdr = (RefCountingDataReader) sqlHelper.ExecuteReader(cmd);

var sqlDr = (SqlDataReader) refCountdr.InnerReader;
if (sqlDr != null) {
  while (sqlDr.Read())    
  {

  }
}

And I also think the looping is repetitive for each method. Each such method has a loop that maps object to cmd.Parameters

How do I separate the looping and mapping? Should I create an extension method on ParkingSlipDetails Class that takes SqlDataReader and returns an instance of ParkingSlipDetails? Even then I will be repeating reader creation and lopping in each model class.

Answer 1

I think this is where LINQ to Sql would come in handy. A simple method like this:

public List<T> getObjects<T>(IDbConnection connection,string tableName,params string[] columnNames)
{
    string query = $@"SELECT {String.Join(",", columnNames)} FROM {tableName}";
    using (var dc = new DataContext(connection))
    {
        return dc.ExecuteQuery<T>(query).ToList();
    }

}

would simplify things greatly. For instance using a simple class:

public class Company
{
    public string CustomerID { get; set; }
    public string CompanyName { get; set; }
    public string Address { get; set; }
}

you could get a list from the Northwinds database like this:

OleDbConnection conn = new OleDbConnection("Provider=Microsoft.Jet.OLEDB.4.0;Data Source=NWind.mdb");
var companyList = getObjects<Company>(conn,"Customers","CustomerID","CompanyName","Address");

This requires that your properties be properly typed and that the names match the column names you need(they can be case insensitive though). If conversions are needed you can leverage the setter for that.

Depending on what you need to do with the data, you might find it easier to create a DataTable instead of a List. This will automatically get the names and the types properly set, while still allowing you to iterate over the data and access any data item you need.

Answer 2

To quote Mark Seemann from the following article Small methods are easy to troubleshoot

Write small methods. How small? Small enough that any unhandled exception is easy to troubleshoot.

Break the function up into smaller more manageable chunks that focus on a single concern as much as possible.

Yes, the net effect of this is a lot of additional functions. They are however easier to read and maintain.

Another benefit of the many small methods it that it would help identify areas that can be generalized into reusable services that help reduce repeated code, allowing it (the code) to be more DRY (Don't Repeat Yourself)

Lets start with getting the connection

private SqlConnection createConnection() {
    var connection = (SqlConnection) sqlHelper.CreateConnection(); //sqlHelper is a nugget and not in my
    connection.Open();
    return connection;
}

This can now be wrapped in a using block that will dispose of the connection once out of scope

using(SqlConnection connection = createConnection()) {

    //...
}

While there is nothing wrong with creating the command manually, I personal prefer to let the connection do it rather than newing one. Again wrapping in a using block, and populating.

//...

using(SqlCommand command = connection.CreateCommand()) {
    command.CommandText = Constants.GetAllParkingSlips;
    command.CommandType = System.Data.CommandType.StoredProcedure;
    command.Parameters.Add("@CreatedBy", SqlDbType.NVarChar).Value = gepservice.GetUserContext().UserId;
    command.Parameters.Add("@OffsetRows", SqlDbType.NVarChar).Value = offset;
    command.Parameters.Add("@FetchRows", SqlDbType.NVarChar).Value = fetch;

    //...
}

That could have also been refactored into its own method, but in my opinion it would have ended up with too many arguments.

This could be fixed with an aggregated value object.

public class ParkingSlipQuery {
    public int FetchRows { get; set; }
    public int OffsetRows  { get; set; }
    public string CreatedBy { get; set; }
}

for example

private SqlCommand createCommand(SqlConnection connection, ParkingSlipQuery query) {
    SqlCommand command = connection.CreateCommand();
    command.CommandText = Constants.GetAllParkingSlips;
    command.CommandType = System.Data.CommandType.StoredProcedure;
    command.Parameters.Add("@CreatedBy", SqlDbType.NVarChar).Value = query.CreatedBy;
    command.Parameters.Add("@OffsetRows", SqlDbType.NVarChar).Value = query.OffsetRows;
    command.Parameters.Add("@FetchRows", SqlDbType.NVarChar).Value = query.FetchRows;
    return command;
}

and implemented

var query = new ParkingSlipQuery {
    CreatedBy = gepservice.GetUserContext().UserId,
    OffsetRows = offset,
    FetchRows = fetch
};
using(SqlCommand command = createCommand(connection, query)) {
    //...
}

The execution of the command and subsequent reader result however can be moved

private SqlDataReader executeReader(SqlCommand command) {
    var refCountdr = (RefCountingDataReader) sqlHelper.ExecuteReader(cmd);
    return (SqlDataReader) refCountdr.InnerReader;
}

As you have realized by now, you should know what is coming next about building the model to populate the collection.

private ParkingSlipDetails getDetails(SqlDataReader reader) {
    ParkingSlipDetails details = new ParkingSlipDetails();
    details.Id = Convert.ToInt32(reader[FileUploadConstants.Id]);
    details.FileName = Convert.ToString(reader[FileUploadConstants.FileName]);
    details.FileUri = Convert.ToString(reader[FileUploadConstants.FileURI]);
    details.PONumber = Convert.ToString(reader[FileUploadConstants.DocumentNumber]);
    details.UploadDate = Convert.IsDBNull(reader[FileUploadConstants.UploadDate]) ? DateTime.UtcNow: Convert.ToDateTime(reader[FileUploadConstants.UploadDate]);
    details.FileStatus = (Status)(Convert.ToInt16(reader[FileUploadConstants.FileStatus]));
    details.ReceiptNumber = Convert.ToString(reader[FileUploadConstants.RecieptNumber].ToString());
    details.TotalRows = Convert.ToInt32(reader[FileUploadConstants.TotalRows]);
    details.CreatedBy = gepservice.GetUserContext().UserId;

    return details;
}

private List<ParkingSlipDetails> getDetailsList(SqlDataReader reader, string userId) {
    List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
    if (reader != null) {
        while (reader.Read()) {
            ParkingSlipDetails details = getDetails(reader);
            details.CreatedBy = userId;
            detailsList.Add(details);
        }
    }
    return detailsList;
}

Finally, to avoid null reference errors, the method should try to avoid return null. An empty collection is safer to check than a null collection.

This results in refactor looking like

public CustomReturn<List<ParkingSlipDetails>> GetAllParkingSlips(int offset, int fetch) {
    List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
    try {
        using(SqlConnection connection = createConnection()) {
            string userId = gepservice.GetUserContext().UserId;
            var query = new ParkingSlipQuery {
                CreatedBy = userId,
                OffsetRows = offset,
                FetchRows = fetch
            };
            using(SqlCommand command = createCommand(collection, query)) {
                using(SqlDataReader reader = executeReader(command)) {
                    detailsList = getDetailsList(reader, userId);
                }
            }
        }
    } catch(Exception ex) {
        LogError("ManageParkingSlipDataAccess", "GetAllParkingSlips", "Error", ex);
        detailsList = new List<ParkingSlipDetails>();
    }
    return new CustomReturn<List<ParkingSlipDetails>>(detailsList);
}

The using block will handle the closing and disposal of the connect and the other disposable members.

If following explicit dependency principle, it could be refactored further down by delegating the argument values to the caller.

public CustomReturn<List<ParkingSlipDetails>> GetAllParkingSlips(ParkingSlipQuery query) {
    List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
    try {
        using(SqlConnection connection = createConnection()) {                
            using(SqlCommand command = createCommand(collection, query)) {
                using(SqlDataReader reader = executeReader(command)) {
                    detailsList = getDetailsList(reader, query.CreatedBy);
                }
            }
        }
    } catch(Exception ex) {
        LogError("ManageParkingSlipDataAccess", "GetAllParkingSlips", "Error", ex);
        detailsList = new List<ParkingSlipDetails>();
    }
    return new CustomReturn<List<ParkingSlipDetails>>(detailsList);
}

with the following support methods

private SqlConnection createConnection() {
    var connection = (SqlConnection) sqlHelper.CreateConnection(); //sqlHelper is a nugget and not in my
    connection.Open();
    return connection;
}

private SqlCommand createCommand(SqlConnection connection, ParkingSlipQuery query) {
    SqlCommand command = connection.CreateCommand();
    command.CommandText = Constants.GetAllParkingSlips;
    command.CommandType = System.Data.CommandType.StoredProcedure;
    command.Parameters.Add("@CreatedBy", SqlDbType.NVarChar).Value = query.CreatedBy;
    command.Parameters.Add("@OffsetRows", SqlDbType.NVarChar).Value = query.OffsetRows;
    command.Parameters.Add("@FetchRows", SqlDbType.NVarChar).Value = query.FetchRows;
    return command;
}

private SqlDataReader executeReader(SqlCommand command) {
    var refCountdr = (RefCountingDataReader) sqlHelper.ExecuteReader(cmd);
    return (SqlDataReader) refCountdr.InnerReader;
}

private List<ParkingSlipDetails> getDetailsList(SqlDataReader reader, string userId) {
    List<ParkingSlipDetails> detailsList = new List<ParkingSlipDetails>();
    if (reader != null) {
        while (reader.Read()) {
            ParkingSlipDetails details = getDetails(reader);
            details.CreatedBy = userId;
            detailsList.Add(details);
        }
    }
    return detailsList;
}

private ParkingSlipDetails getDetails(SqlDataReader reader) {
    ParkingSlipDetails details = new ParkingSlipDetails();

    details.Id = Convert.ToInt32(reader[FileUploadConstants.Id]);
    details.FileName = Convert.ToString(reader[FileUploadConstants.FileName]);
    details.FileUri = Convert.ToString(reader[FileUploadConstants.FileURI]);
    details.PONumber = Convert.ToString(reader[FileUploadConstants.DocumentNumber]);
    details.UploadDate = Convert.IsDBNull(reader[FileUploadConstants.UploadDate]) ? DateTime.UtcNow: Convert.ToDateTime(reader[FileUploadConstants.UploadDate]);
    details.FileStatus = (Status)(Convert.ToInt16(reader[FileUploadConstants.FileStatus]));
    details.ReceiptNumber = Convert.ToString(reader[FileUploadConstants.RecieptNumber].ToString());
    details.TotalRows = Convert.ToInt32(reader[FileUploadConstants.TotalRows]);

    return details;
}

Answer 3

If LINQ-to-SQL and other ORMs are not possible to use in your organization. I would suggest to create a fixable solution that makes things easier for you for future updates. For instance, you could create a new class to handle SQL operations. This class will be a thin layer on top of the existing layer. Then, use it to design new methods that will reduce code redundancy, and recall them from the current ones. Then over the time, you'll see that this new handler will replace the current work, which will make your life easier.

DataTable also another approach, but it depends on the server IO and how much data it's going to be processed, which consumes more memory.

for your question, you can do something like this :

public IEnumerable<IDataRecord> Reader(string query,  SqlParameter[] parameters = null, CommandType commandType = CommandType.StoredProcedure)
{
    var globalConnection = (SqlConnection) sqlHelper.CreateConnection(); //sqlHelper is a nugget and not in my control

    using(var connection = new SqlConnection(globalConnection.ConnectionString)) // just create this scope, in case you don't want to the global connection to be disposed.
    using (var cmd = new SqlCommand(query, connection) { CommandType = commandType })
    {
        // if there is parameters add them
        if (parameters != null)
        {
            for (int x = 0; x < parameters.Length; x++)
            {
                cmd.Parameters.Add(parameters[x]);
            }
        }

        connection.Open();

        using (SqlDataReader reader = cmd.ExecuteReader())
        {
            if (reader.HasRows)
            {
                foreach (var row in reader)
                {
                    yield return (DbDataRecord)(IDataRecord)row;
                }
            }
        }

    } // only this connection will be disposed
}

and then use it like this :

public CustomReturn<List<ParkingSlipDetails>> GetAllParkingSlips(int offset, int fetch)
{
    try
    {
        var ParkingSlipDetailsList = new List<ParkingSlipDetails>();


    SqlParameter[] parameters = new SqlParameter[]
    {
        new SqlParameter{ ParameterName = "@CreatedBy", SqlDbType = SqlDbType.NVarChar, Value = gepservice.GetUserContext().UserId},
        new SqlParameter{ ParameterName = "@OffsetRows", SqlDbType = SqlDbType.NVarChar, Value = offset},
        new SqlParameter{ ParameterName = "@FetchRows", SqlDbType = SqlDbType.NVarChar, Value = fetch}
    };



        var reader = Reader(Constants.GetAllParkingSlips, parameters);

        foreach(var row in reader)
        {

            var pSlips = new ParkingSlipDetails();

            for(int x =0; x < pSlips.GetType().GetProperties().Length; x++)
            {
                var column_name  = row.GetName(x);
                var column_type  = row.GetFieldType(x);
                var column_value = row.GetValue(x);

                // for columns that don't have identical naming with the model's properties 
                if(column_name.Equals(FileUploadConstants.FileURI, StringComparison.OrdinalIgnoreCase))
                {
                    column_name = "FileUri";
                }
                else if(column_name.Equals(FileUploadConstants.DocumentNumber, StringComparison.OrdinalIgnoreCase))
                {
                    column_name = "PONumber";
                }
                else if(column_name.Equals(FileUploadConstants.RecieptNumber, StringComparison.OrdinalIgnoreCase))
                {
                    column_name = "ReceiptNumber";
                }
                else if(column_name.Equals(FileUploadConstants.UploadDate, StringComparison.OrdinalIgnoreCase))
                {
                    column_value = Convert.IsDBNull(sqlDr[FileUploadConstants.UploadDate]) ? DateTime.UtcNow: Convert.ToDateTime(sqlDr[FileUploadConstants.UploadDate]);
                }                   
                else if(column_name.Equals("CreatedBy", StringComparison.OrdinalIgnoreCase))
                {
                    column_value = gepservice.GetUserContext().UserId;                      
                }

                    // Use refelection to set the values & use Convert.ChangeType() to convert the data reader value to the property's datatype. 
                    pSlips.GetType().GetProperty(column_name).SetValue(pSlips, Convert.ChangeType(column_value, column_type), null);                                                        


            }


            ParkingSlipDetailsList.Add(pSlips);
        }


        return new CustomReturn<List<ParkingSlipDetails>> (ParkingSlipDetailsList);
    }
    catch(Exception ex)
    {
        LogError("ManageParkingSlipDataAccess", "GetAllParkingSlips", "Error", ex);
        return null;
    }
    finally
    {
        // there is no connection in this scope, since it's already disposed within Reader() method.

        //if (sqlConnection != null)
        //sqlConnection.Close();
    }
}