Repository and Data Mapper pattern

Question

After a lots of read about Repository and Data Mapper I decided to implement those patterns in a test project. Since I'm new to these I'd like to get your views about how did I implement those in a simple project.

Jeremy Miller says :

Do some sort of nontrivial, personal coding project where you can freely experiment with design patterns.

But I don't know I did all this things right or not.

Here is my project structure :

As you can see there are many folders which I'm going to describe them in detail in below.

• Domain : Project Domain Entities go here I've a simple Personnel class which is inherited from EntityBase class, EntityBase class has a single property named Id.

  public int Id { get; set; }
  

• Infrustructure : Here is a simple Data Access Layer with two classes. SqlDataLayer is a simple class which is inherited from an abstract class named DataLayer. Here I provide some functionality like following code :

  public SQLDataLayer() {
      const string connString = "ConnectionString goes here";
      _connection = new SqlConnection(connString);
      _command = _connection.CreateCommand();
  }
  

adding parameter to commands parameter collection :

    public override void AddParameter(string key, string value) {
        var parameter = _command.CreateParameter();
        parameter.Value = value;
        parameter.ParameterName = key;

        _command.Parameters.Add(parameter);
    }

executing DataReader :

    public override IDataReader ExecuteReader() {
        if (_connection.State == ConnectionState.Closed)
            _connection.Open();

        return _command.ExecuteReader();
    }

and so on.

• Repository : Here I tried to implement repository pattern. IRepository is a generic interface

IRepository.cs :

public interface IRepository<TEntity> where TEntity : EntityBase
{
    DataLayer Context { get; }

    TEntity FindOne(int id);
    ICollection<TEntity> FindAll();

    void Delete(TEntity entity);
    void Insert(TEntity entity);
    void Update(TEntity entity);
}

Repository.cs :

public class Repository<TEntity> : IRepository<TEntity> where TEntity : EntityBase, new() {
    private readonly DataLayer _domainContext;
    private readonly DataMapper<TEntity> _dataMapper;
    public Repository(DataLayer domainContext, DataMapper<TEntity> dataMapper) {
        _domainContext = domainContext;
        _dataMapper = dataMapper;
    }
    public DataLayer Context {
        get { return _domainContext; }
    }
    public TEntity FindOne(int id)
    {
        var commandText = AutoCommand.CommandTextBuilder<TEntity>(CommandType.StoredProcedure, MethodType.FindOne);

        // Initialize parameter and their types
        Context.AddParameter("Id", id.ToString(CultureInfo.InvariantCulture));
        Context.SetCommandType(CommandType.StoredProcedure);
        Context.SetCommandText(commandText);

        var dbReader = Context.ExecuteReader();
        return dbReader.Read() ? _dataMapper.Map(dbReader) : null;
    }

I didn't expose not implemented methods from IRepository.

Here in Generic Repository class I expect two parameters in constructor first is a reference to my SqlDataLayer class and second is a reference to Entity DataMapper. Those parameters sent by each Entities Repository class which inherited from Repository class. for example :

public class PersonnelRepository : Repository<Personnel>, IPersonnelRepository {
    public PersonnelRepository(DataLayer domainContext, PersonnelDataMapper dataMapper)
        : base(domainContext, dataMapper) {

    }
}

As you can see here in the FindOne method I tried to automate some operation such as creating CommandText, then I took the advantage of my DataLayer class to configure command and finally execute command to get IDataReader. I pass IDataReader to my DataMapper class to map to the Entity.

• DomainMapper : Finally here I map result of IDataReader to Entities, bellow is a sample of how I map Personnel entity :

  public class PersonnelDataMapper : DataMapper<Personnel> {
  public override Personnel Map(IDataRecord record) {
      return new Personnel {
          FirstName = record["FirstName"].ToString(),
          LastName = record["LastName"].ToString(),
          Address = record["Address"].ToString(),
          Id = Convert.ToInt32(record["Id"])
      };
  }}
  

Usage :

    using (var context = new SQLDataLayer()) {
        _personnelRepository = new PersonnelRepository(context, new PersonnelDataMapper());
            var personnel  = _personnelRepository.FindOne(1);
    }

I know I did many mistake here, that's why I'm here. I need your advice to know what I did wrong or what are the good points in this simple test project.

Thanks in advance.

Answer 1

A few points:

1. It strikes me that overall, you have a good design. That's evidenced, in part, by the fact that you can make changes in it with little impact on any classes outside of the ones that are changed (low coupling). That said, it's very close to what Entity Framework does, so while it's a good personal project, I'd consider using EF first before implementing it in a production project.

2. Your DataMapper class could be made generic (say, GenericDataMapper<T>) using reflection. Iterate over the properties of type T using reflection, and get them from the data row dynamically.

3. Assuming you do make a Generic DataMapper, you could consider making a CreateRepository<T>() method on DataLayer, so that users don't need to worry about the details of which type of Mapper to pick.

4. A minor critique- you assume that all entities will have a single integer ID named "Id", and that a stored procedures will be set up to retrieve them by such. You may be able to improve your design here by allowing for primary keys of differing types, again maybe by using generics.

5. You probably don't want to re-use Connection and Command objects the way you do. That isn't thread safe, and even if it was, you'd end up with some surprising and hard-to-debug race conditions around DB Transactions. You either should create new Connection and Command objects for each function call (making sure to dispose of them after you are done), or implement some synchronization around the methods that access the database.

For instance, I'd suggest this alternate version of ExecuteReader:

public override IDataReader ExecuteReader(Command command) {
    var connection = new SqlConnection(connString);
    command.Connection = connection;
    return command.ExecuteReader();
}

Your old one re-used the command object, which could lead to race conditions between multithreaded callers. You also want to create a new connection, because the old connection might be engaged in a transaction started by a different caller. If you want to re-use transactions, you should create a connection, begin a transaction, and re-use that transaction until you have executed all of the commands that you want to associate with the transaction. As an example, you could create overloads of your ExecuteXXX methods like this:

public override IDataReader ExecuteReader(Command command, ref SqlTransaction transaction) {
    SqlConnection connection = null;
    if (transaction == null) {
        connection = new SqlConnection(connString);
        transaction = connection.BeginTransaction();
    } else {
        connection = transaction.Connection;
    }
    command.Connection = connection;
    return command.ExecuteReader();
}    

// When you call this, you can pass along a transaction by reference.  If it is null, a new one will be created for you, and returned via the ref parameter for re-use in your next call:

SqlTransaction transaction = null;

// This line sets up the transaction and executes the first command
var myFirstReader = mySqlDataLayer.ExecuteReader(someCommandObject, ref transaction);

// This next line gets executed on the same transaction as the previous one.
var myOtherReader = mySqlDataLayer.ExecuteReader(someOtherCommandObject, ref transaction);

// Be sure to commit the transaction afterward!
transaction.Commit();

// Be a good kid and clean up after yourself
transaction.Connection.Dispose();
transaction.Dispose();

6. Last but not least, having worked with Jeremy I'm sure he'd say that you should have unit tests for all of these classes!