Refactoring the following methods to remove duplicate code

Question

I have a bunch of DAO classes that do something similar for an Entity. I was wondering if anyone can help me with ways to: 1) Simplify this code 2) Stop from duplicating code like this for each entity.

    public IList<IUser> GetAll()
    {
        IList<IUser> users = new List<IUser>();

        using (var myConnection = new SqlConnection(ApplicationConfig.ConnectionString))
        {
            using (var myCommand = new SqlCommand("sp_GetAllUsers", myConnection))
            {
                myCommand.CommandType = CommandType.StoredProcedure;
                myConnection.Open();
                using (SqlDataReader myReader = myCommand.ExecuteReader())
                {
                    if (myReader != null)
                    {
                        while (myReader.Read())
                            users.Add(FillDataRecord(myReader));
                        myReader.Close();
                    }
                }
            }
            myConnection.Close();
        }
        return users;
    }

    public IUser GetBy(int id)
    {
        IUser user = null;

        using (var myConnection = new SqlConnection(AppConfiguration.ConnectionString))
        {
            using (var myCommand = new SqlCommand("sp_GetUserById", myConnection))
            {
                myCommand.Parameters.Add("@id", System.Data.SqlDbType.Int).Value = id;
                myCommand.CommandType = CommandType.StoredProcedure;
                myConnection.Open();
                using (SqlDataReader myReader = myCommand.ExecuteReader())
                {
                    if (myReader != null)
                    {
                        while (myReader.Read())
                            user = FillDataRecord(myReader);
                        myReader.Close();
                    }
                }
            }
            myConnection.Close();
        }
        return user;
    }

    private static IUser FillDataRecord(IDataRecord myDataRecord)
    {
        IAddress address = null;

        if (!myDataRecord.IsDBNull(myDataRecord.GetOrdinal("AddressId")))
            address= GetAddress(myDataRecord.GetInt32(myDataRecord.GetOrdinal("AddressId")));

        IUser user = new User
                                   {
                                        Id = myDataRecord.GetInt32(myDataRecord.GetOrdinal("Id"))
                                       ,LastName = myDataRecord.IsDBNull(myDataRecord.GetOrdinal("LastName")) ? string.Empty : myDataRecord.GetString(myDataRecord.GetOrdinal("LastName"))
                                       ,FirstName = myDataRecord.IsDBNull(myDataRecord.GetOrdinal("FirstName")) ? string.Empty : myDataRecord.GetString(myDataRecord.GetOrdinal("FirstName"))
                                       ,MyAddress= address
                                   };
        return user;
    }

Thanks in advance

Answer 1

Use generics and a base class that all your data objects derive from. Here's what I did:

using System;
using System.Collections.Generic;

namespace StreamSubServer
{


    public abstract class DataObjectBase<T>
    {
     public List<T> GetAll()
     {
      //get the value of the StoredProcedureAttribute attribute

     }
     public T GetBy(int id)
     {
      ...
     }
    }
    [StoredProcedure("usp_GetUsers")]
    public class User : DataObjectBase<User>
    {
     string userName {get;set;}

    }
}

Then replace the ellipses with implementation code that uses reflection to walk through the columns returned in your dataset/datareader and assign the values to properties. You can get a better idea of how to do this from the ActiveRecord source code, which I shamelessly cribbed from to make my own data access layer.

Implement the StoredProcedure attribute and use reflection to examine its value, pass that into your fill method and cha-ching!

Good luck!

Answer 2

I think the simplest refactoring is:

public IList<IUser> GetAll()
    {

        using (var myConnection = new SqlConnection(ApplicationConfig.ConnectionString))
        {
            using (var myCommand = new SqlCommand("sp_GetAllUsers", myConnection))
            {
               return loadUserList(myConnection, myCommand);
            }
            myConnection.Close(); // This should be in a finally if to be used
        }
        return null;
    }
private IList<IUser> loadUserList(SqlConnection myConnection, SqlCommand myCommand) {
    IList<IUser> users = new List<IUser>();
            myCommand.CommandType = CommandType.StoredProcedure;
            myConnection.Open();
            using (SqlDataReader myReader = myCommand.ExecuteReader())
            {
                if (myReader != null)
                {
                    while (myReader.Read())
                        users.Add(FillDataRecord(myReader));
                    myReader.Close();
                }
            }
     return users;
 }

If the other function you can return just the first record, to return a since IUser, if you desire.

To make this more general, pass in an action to do the processing, so each entity can be responsible for it's own parsing, and use generics as was already mentioned.