Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to design a well defined yet simple interface for the unit of work and repository patterns. My UoW's are exposed to services and services then "get repositories" that it needs to query. I know returning IQueryable<T> for repositories is a religious war. Because repositories are only exposed to the service, all queries are performed inside the service and therefore I can test the queries. Is there anything I should change for these interfaces? All criticisms are greatly appreciated!

public interface IUnitOfWork : IDisposable
{
    bool IsActive { get; }

    bool WasCommitted { get; }

    /// <summary>
    /// Commits all changes made on the unit of work.
    /// </summary>
    void Commit();

    bool WasRolledBack { get; }

    /// <summary>
    /// Rolls back all changes made on the unit of work.
    /// </summary>
    void Rollback();

    /// <summary>
    /// Returns an instance of an entity with the specified key that is attached to the unit of work without
    /// loading the entity from a repository.
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    T Load<T>(int id)
        where T : class;

    void Attach<T>(T entity)
        where T : class, IIdentifiable;

    void Detach<T>(T entity)
        where T : class;

    IRepository<T> GetRepository<T>()
        where T : class;
}

public interface IRepository<T>
    where T : class
{

    IUnitOfWork UnitOfWork { get; }

    void Add(T entity);

    void Remove(T entity);

    /// <summary>
    /// Returns an instance of an entity with the specified key that is attached to the unit of work by loading
    /// the entity from the repository.
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    T Get(int id);

    IQueryable<T> All();
}
share|improve this question
add comment

4 Answers

I think your interfaces are mostly well-defined, with a couple exceptions:

I don't think Load, Attach and Detach should be members of IUnitOfWork, but rather IRepository. It seems that the repository that manages objects of type T would be the best place to place methods that acts on that type.

The UnitOfWork property on IRepository does not belong there. Things that modify one repository shouldn't be allowed to go and get other repositories to modify on their own. If they need them, pass them in explicitly. Otherwise, you're hiding from potential callers the fact that your implementation actually depends on more than just the one repository, and hiding your dependencies like you're ashamed of them is one of my least favorite code smells.

share|improve this answer
    
Good point about the UoW property, it is completely useless and I forgot I still had it there. The reason Attach and Detach are on the UoW is because you're not working with the repository per se: it's saying "ok stop tracking changes" and "ok track changes again". Load on the other hand, I'm not so sure about now. Do you have any other bullets against this one? I can see moving it to the repository sort of making sense (it is working with an object - but will not have its graph loaded). –  TheCloudlessSky Jan 29 '11 at 14:21
    
Also - What do you think about the WasCommitted etc. Do you think they're necessary? –  TheCloudlessSky Jan 29 '11 at 14:22
add comment

Here is how I would shape it:

public interface IEntity
{
    GUID Id { get; }
}

public interface IUnitOfWork<TEntity> where TEntity : IEntity, class 
    : IDisposable
{
    void Commit();
    void Discard();
    void Track(TEntity entity);
    void Delete(TEntity entity);
}

public interface IRepository<TEntity> where TEntity : IEntity, class 
    : IDisposable
{
    IUnitOfWork<TEntity> UnitOfWork { get; }

    TEntity CreateNew();
    T FindOne(ISpecification criteria);
    IEnumerable<T> FindMandy(ISpecification criteria);
    IEnumerable<T> FetchAll();
}

public interface ISpecification
{
    // encapsulates everything what is needed for a query 
    // this might be an SQL-statement, LINQ-functor... 

    string CommandText { get; }
    object[] Arguments { get; set; }
    bool IsSatisfiedBy(object candidate);
}

Repository itself is read only, UnitOfWork is writeonly. I can't see how this design breaks Law of Demeter.

share|improve this answer
add comment

Your interface breaks the Law of Demeter. Or said simply I cannot use IUnitOfWork UnitOfWork { get; } on IRespository without understanding IUnitOfWork.

I would change IRespository too

public interface IRepository<T>
    where T : class
{

    bool IsActive { get; }

    bool WasCommitted { get; }

    /// <summary>
    /// Commits all changes made on the unit of work.
    /// </summary>
    void Commit();

    bool WasRolledBack { get; }

    /// <summary>
    /// Rolls back all changes made on the unit of work.
    /// </summary>
    void Rollback();

    /// <summary>
    /// Returns an instance of an entity with the specified key that is attached to the unit of work without
    /// loading the entity from a repository.
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    T Load<T>(int id)
        where T : class;

    void Attach<T>(T entity)
        where T : class, IIdentifiable;

    void Detach<T>(T entity)
        where T : class;

    IRepository<T> GetRepository<T>()
        where T : class;

    void Add(T entity);

    void Remove(T entity);

    /// <summary>
    /// Returns an instance of an entity with the specified key that is attached to the unit of work by loading
    /// the entity from the repository.
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    T Get(int id);

    IQueryable<T> All();
}

Notice how it is a copy of IUnitOfWork, is UnitOfWork really needed.

How did you create the interface?

This is where you should do TDD. Write the test first about what you want the code to do. Then your interface should be created in order to accomplish the test.

For example if you call the method Rollback(); and cannot write a test were WasRollbacked property was used, they you might not need the property.

Do not put in more than needed to any interface. They interface becomes noise. I would recommend reading Clean Code by Robert Martin to understand more of these ideas in a in depth way.

share|improve this answer
1  
I've read clean code. I'm not really sure how this even makes sense. The repository pattern is strictly responsible for being an interface to the data layer. Having IUnitOfWork exposed as a property on the repository might not really make sense, but I had just left it there. Putting these methods on the repository interface just doesn't make sense. –  TheCloudlessSky Jan 27 '11 at 18:48
add comment

I don't remember exactly where I got my implementation, but a lot of it is borrowed from the Spring Framework...

The only three methods that my IUnitOfWork interface exposes are Initialize(), Commit(), and Rollback(). Then I inherit from that for an INhibernateUnitOfWork, which simply exposes the NHibernate Session via a property as well... I guess this part may depend on what you are using for the persistence storage mechanism...

But what it allows me to do in my repository class then is to simply take in the unit of work as an injectable dependency. Essentially, each of my Get(id), Load(id), Find(), Save(), Update(), etc. methods just make a call against the Repository.UnitOfWork.Session property.

In essence, it's really close to what you've already implemented... Not sure why you want the GetRepository method, but you may have some use case for that which I do not. For me, my UnitOfWork is Request lifetime object whose Initialize() and Commit() / Rollback() functionality is handled by an HTTP module hooking the BeginRequest and EndRequest events. That is, the UnitOfWork instance is only ever touched by said HTTP Module, and by the repositories that simply use it to get a reference to the current Session object.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.