Refactoring to avoid duplicate code

I'm trying to factor out some repetitive code, but it starts to smell funky now. Say I start out with this not quite right, but you catch my drift:

    public virtual OrganisationEntity Get(int id)
    {
        SqlCommand command = new SqlCommand();
        command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id";
        command.Parameters.Add("@id", SqlDbType.Int).Value = id;

        List<OrganisationEntity> entities = new List<OrganisationEntity>();

        SqlDataReader reader = Database.ExecuteQuery(command, ConnectionName.Dev);

        while (reader.Read())
        {
            OrganisationEntityMapper mapper = Mapper;
            entities = mapper.MapAll(reader);
        }

        return entities.First<OrganisationEntity>();
    }

It's pretty obvious every other Get(int id) methode has, apart from the query the same form, so my next step would be to make a base class, RepositoryBase looking like:

public abstract class RepositoryBase<T> where T : new()
{
    /// <summary>
    /// 
    /// </summary>
    public abstract EntityMapperBase<T> Mapper { get; }

    public virtual T Get(int id)
    {

        List<T> entities = new List<T>();

        SqlDataReader reader = Database.ExecuteQuery(Command, ConnectionName);

        while (reader.Read())
        {
            EntityMapperBase<T> mapper = Mapper;
            entities = mapper.MapAll(reader);
        }

        return entities.First<T>();
    }
}

To add some generic funkyness, but this is also where it gets ugly. First of all, Database.ExecuteQuery expects a SqlCommand and an enum, so I though, ok, then I'll add 2 properties, which I'll just fire up with some stuff. THen I realize I don't need the int id parameter here anymore, since I construct the query in a subclass, so I might as well pass the command and connectionName as parameters, I want the connectionName to be dependend of the OrganisationRepository anyway (others need another string):

public class OrganisationRepository : RepositoryBase<OrganisationEntity>
{
    protected override EntityMapperBase<OrganisationEntity> Mapper
    {
        get
        {
            return new OrganisationMapper();
        }            
    }

    public override OrganisationEntity Get(int id)
    {
        SqlCommand command = new SqlCommand();
        command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id";
        开发者_开发技巧command.Parameters.Add("@id", SqlDbType.Int).Value = id;
        return base.Get(command, ConnectionName.Dev);
    }
}

But, oops, ofcourse, now the method signatures aren't in sync anymore... oops! So, basically I'm wondering. It just feels nasty, but don't know exactly why. On one hand I'd like to factor out repetetive code as much as possible, but now it leaves me with this!

How do I refactor this to (more) proper OO? Should I just forget factoring out the query strings and write alot of duplicate?

Your "next step" would not be the same as mine.

My next step would be to find another example of this "common code" you're trying to refactor. Perhaps a "`CustomerEntity.Get(int id)'" method.

Now, let's pretend the only difference between the CustomerEntity and OrganisationEntity versions are the query string and the replacement of the term "Organisation" with "Customer". My next step would be to try to make the two methods more and more identical. Assuming this method is part of an OrganisationEntityRepository class, I'd refactor that towards an EntityRepository1 class, and the CustomerEntityRepository towards EntityRepository2.

Steps 1 would be to introduce a generic parameter for the type of the entity. You'll have to do the same for the OrganisationEntityMapper and CustomerEntityMapper classes.

Next, go back and look at what's still different. I see they use different mapper classes, so let's make the mapper type generic. In order to do that and still reference the MapAll method, I'll introduce an IMapper interface with the MapAll method, and have my two concrete mapper classes implement that.

Now, the next big difference is the query. I'll put that into a virtual "CommandText" property.

Now I think I'm ready for a base class, perhaps EntityRepositoryBase<TEntity,TMapper>. With suitable assumptions, I wind up with the following:

public abstract class EntityRepositoryBase<TEntity, TMapper>
    where TMapper : IMapper<TEntity>
{
    public virtual TEntity Get(int id)
    {
        List<TEntity> entities;
        using (var command = new SqlCommand {CommandText = CommandText})
        {
            command.Parameters.Add("@id", SqlDbType.Int).Value = id;

            entities = new List<TEntity>();

            using (var reader = Database.ExecuteQuery(command, ConnectionName.Dev))
            {
                while (reader.Read())
                {
                    var mapper = Mapper;
                    entities = mapper.MapAll(reader);
                }
            }
        }

        return entities.First();
    }

    protected abstract string CommandText { get; }
    protected abstract TMapper Mapper { get; }
}

public class OrganisationEntityRepository :
    EntityRepositoryBase<OrganisationEntity, OrganisationEntityMapper<OrganisationEntity>>
{
    protected override string CommandText
    {
        get { return @"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id"; }
    }

    protected override OrganisationEntityMapper<OrganisationEntity> Mapper
    {
        get { throw new NotImplementedException(); }
    }
}

public class CustomerEntityRepository : EntityRepositoryBase<CustomerEntity, CustomerEntityMapper<CustomerEntity>>
{
    protected override string CommandText
    {
        get { return @"SELECT t.Id, t.Description FROM Customer t Where t.Id = @Id"; }
    }

    protected override CustomerEntityMapper<CustomerEntity> Mapper
    {
        get { throw new NotImplementedException(); }
    }
}

And, needless to say, though I'll say it anyway: props to JetBrains ReSharper 5.1 for doing all the moving around of things, so I didn't have to.

I believe ORMs like Entity Framework or nHibernate would be more suitable solution to this situation. They can take care of all the plumbing that you are trying to build by yourself.

If you can use an ORM framework in your project, I think that would be preferable instead of writing all this by hand.

However, one way you can do this refactoring is to have an abstract method in RepositoryBase having this signature:

public abstract T Get(int id);

and also a protected method with this signature:

protected T Get(SqlCommand command, SqlConnection connection)

that has the same code you showed previously in RepositoryBase.

This way, in derived classes you only need to implement the one where you construct the command and call the one from the base class that does the actual database call.

I would start off with the following. Define the Database and Mapping functionality in separate interfaces and inject them into the repository, this way the repository will be easier to test. Using this method you should be able to extend the repository to include other CRUD operations.

One problem with this approach is the separation between the mapper and the creation of the SqlCommand, it may not be very obvious which columns are returned via the select statement.

// The concrete implementation of this interface will handle connections to the
// database
public interface IDatabase
{
    SqlDataReader ExecuteQuery(SqlCommand command);
}

public interface IEntityMapper<T>
{
    T MapAll(SqlDataReader reader);
}

public abstract class EntityRepository<T>
{
    private readonly IDatabase _database;
    private readonly IEntityMapper<T> _mapper;

    protected EntityRepository(IEntityMapper<T> mapper, IDatabase database)
    {
        _mapper = mapper;
        _database = database;
    }

    public T Get(int id)
    {
        return this.Get(_mapper, _database, id);
    }

    protected virtual T Get(IEntityMapper<T> mapper, IDatabase database, int id)
    {
        // Create a command can be used to fetch the entity, remember to dispose when complete
        using (var cmd = this.CreateGetCommand(id))
        {
            using (var reader = database.ExecuteQuery(cmd))
            {
                // No need to read all the rows, just the first...
                return reader.Read() ? mapper.MapAll(reader) : default(T);
            }
        }
    }

    protected abstract SqlCommand CreateGetCommand(int id);
}

And implement the following

public class OrganisationEntityRepository : EntityRepository<OrganisationEntity>
{
    public OrganisationEntityRepository(IEntityMapper<OrganisationEntity> mapper, IDatabase database) : base(mapper, database)
    {
    }

    protected override SqlCommand CreateGetCommand(int id)
    {
        var command = new SqlCommand(@"SELECT t.Id, t.Description FROM Organisation t Where t.Id = @Id");
        command.Parameters.Add("@id", SqlDbType.Int).Value = id;

        return command;
    }
}

public class OrganisationEntityMapper : IEntityMapper<OrganisationEntity>
{
    public OrganisationEntity MapAll(SqlDataReader reader)
    {
        return new OrganisationEntity();  // Populate using the reader...
    }
}

Here is how I would refactor it:

public class Repository<T> : IRepository<T> where T : class, new()
{
    private IEntityMapper<T> _mapper;

    public Repository(IEntityMapper<T> mapper)
    {
        _mapper = mapper;
    }

    public virtual T Find(string value)
    {
        SqlCommand command = new SqlCommand();
        command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.Description LIKE @value";
        command.Parameters.Add("@value").Value = value + "%";

        SqlDataReader reader = Database.ExecuteQuery(command, ConnectionName.Dev);
        return FillCollection(reader);
    }

    public void T Get(int id)
    {
        SqlCommand command = new SqlCommand();
        command.CommandText = @"SELECT t.Id, t.Description FROM Organisation t Where t.id = @value";
        command.Parameters.Add("@value").Value = id;

        SqlDataReader reader = Database.ExecuteQuery(command, ConnectionName.Dev);
        if (!reader.Read())
            return null;

        T entity = new T();
        _mapper.Map(entity, reader);
        return entity;
    }

    protected IList<T> FillCollection(IDataReader reader)
    {
        List<T> items = new List<T>();
        while (reader.Read())
        {
            T entity = new T();
            _mapper.Map(entity, reader);
            _items.Add(entity);
        }
        return items;
    }
}



public interface IEntityMapper<T>
{
    //row is the most generic part of a DataReader
    void Map(T entity, IDataRow row);
}

Keypoints:

1. Created a base class with FillCollection as a protected method.
2. Forced entity to be a class with a default constructor to speed up object creation.
3. Using a interface for the mapper and taking it in the constructor.
4. I would not fill an entire collection to just get the first item. waste of cpu.
5. Try to use as generic interfaces as possible, as IDataRecord instead of DbDataReader