24
\$\begingroup\$

Given my IUnitOfWork interface

using System;

public interface IUnitOfWork : IDisposable
{
    void Commit();
}

I then create an abstract factory interface called IUnitOfWorkFactory

using System.Transactions;

public interface IUnitOfWorkFactory
{
    IUnitOfWork GetUnitOfWork(IsolationLevel isolationLevel);
}

I then create a default implementation of my IUnitOfWork called TransactionScopeUnitOfWork

using System;
using System.Transactions;

public class TransactionScopeUnitOfWork : IUnitOfWork
{
    private bool disposed = false;

    private readonly TransactionScope transactionScope;

    public TransactionScopeUnitOfWork(IsolationLevel isolationLevel)
    {
        this.transactionScope = new TransactionScope(
                TransactionScopeOption.Required,
                new TransactionOptions
                {
                    IsolationLevel = isolationLevel,
                    Timeout = TransactionManager.MaximumTimeout
                });
    }

    public void Dispose()
    {
        this.Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                this.transactionScope.Dispose();
            }

            disposed = true;
        }
    }

    public void Commit()
    {
        this.transactionScope.Complete();
    }
}

I then create the factory to return that implementation called TransactionScopeUnitOfWorkFactory

using System.Transactions;

public class TransactionScopeUnitOfWorkFactory : IUnitOfWorkFactory
{
    public IUnitOfWork GetUnitOfWork(IsolationLevel isolationLevel)
    {
        return new TransactionScopeUnitOfWork(isolationLevel);
    }
}

The reason for creating the factory is to allow DI (Dependency Injection) frameworks to use different unit of work implementations depending on configuration.

If TransactionScopeUnitOfWorkFactory was mapped to IUnitOfWorkFactory in a DI container, some sample code for using it in an application could be:

public class Test
{
    private readonly IUnitOfWorkFactory unitOfWorkFactory;

    private readonly IRepository testRepository;

    public Test(
        IRepository testRepository,
        IUnitOfWorkFactory unitOfWorkFactory)
    {
         this.testRepository = testRepository;
         this.unitOfWorkFactory = unitOfWorkFactory;

         using (IUnitOfWork unitOfWork = this.unitOfWorkFactory.GetUnitOfWork(IsolationLevel.Serializable))
        {
            this.testRepository.Delete(1); // Some valid CRUD
            unitOfWork.Commit();
        }
    }

I am asking if this seems like a good implementation. Am I missing anything? I want an IUnitOfWork interface that I can use across applications and not worry about maintaining later on. Any opinions?

\$\endgroup\$
  • 6
    \$\begingroup\$ Welcome to Code Review! That's a great first question, I hope you get some good reviews! \$\endgroup\$ – Phrancis Jan 15 '15 at 15:58
14
\$\begingroup\$

Leaky Abstractions

public interface IUnitOfWork : IDisposable
{
    void Commit();
}

I like that there's only a Commit method here - it makes the IUnitOfWork interface very well segregated/focused. However the interface specifying that all implementations must also implement IDisposable, is a leaky abstraction - you have a specific implementation in mind, and that implementation is leaking into the abstraction: what if I wanted a mock-up implementation that doesn't need to be disposed?

public interface IUnitOfWorkFactory
{
    IUnitOfWork GetUnitOfWork(IsolationLevel isolationLevel);
}

GetUnitOfWork makes it sound like the "factory" is hiding a Singleton or something (the name is reminiscent of "GetInstance") - I like my abstract factories when they expose a single parameterless Create method, and intake their dependencies from the constructor; IsolationLevel is also an implementation detail that's leaking into an abstraction - one specific implementation requires an "isolation level", and that implementation detail is leaked into the abstraction.

I would move : IDisposable to TransactionScopeUnitOfWork, because that specific implementation must implement IDisposable.

Then I'd change the abstract factory interface to look something like this:

public interface IUnitOfWorkFactory
{
    IUnitOfWork Create();
}

And the specific implementation that creates a TransactionScopeUnitOfWork would be responsible for specifying an IsolationLevel:

public class TransactionScopeUnitOfWorkFactory : IUnitOfWorkFactory
{
    private readonly IsolationLevel _isolationLevel;

    public TransactionScopeUnitOfWorkFactory(IsolationLevel isolationLevel)
    {
        _isolationLevel = isolationLevel;
    }

    public IUnitOfWork Create()
    {
        return new TransactionScopeUnitOfWork(_isolationLevel);
    }
}

With IoC Containers such as Ninject, you can easily specify IsolationLevel.Serializable or IsolationLevel.ReadCommitted as needed, depending on whether the factory dependency is being injected into a Smurf or into a FooBar's constructor.

That said I've noticed I seldom need to actually implement an abstract factory with Ninject: you can easily configure DI to inject the result of an anonymous method whenever a class has an IUnitOfWork dependency to be constructor-injected: you just do the binding with .ToFactory and let Ninject do the hard work.

The client code shouldn't be bothered with configuring a transaction isolation level if all it needs to care for is that it's depending on an IUnitOfWork that provides a Commit method; if the client code needs to know about an isolation level, then it knows more than it should about the implementation behind the interface it's presented.

\$\endgroup\$
  • \$\begingroup\$ I agree with many of your points such as changing the factory method name to Create. Spot on. But you mentioned that the client code shouldn't need to know about the transaction isolation level. I think it should. The unit of work is a per transaction action. So I think it is relevant. That is why I put in that flexibility. I didn't hard code it into my container like you suggested. See my example. Also, I put in IDisposable in the IUnitOfWork because I wanted to put in the using block. How would you suggest to use it if it isn't in there? \$\endgroup\$ – Issa Fram Jan 15 '15 at 17:23
  • 1
    \$\begingroup\$ There's a balance between ease of use (i.e. using-friendliness) and how "pure" you want things to be - I'm just pointing out that not all implementations may need to be disposable. As for the isolation level, by changing it on a whim you may be running into unexpected issues unless you're using SQL Server 2014. \$\endgroup\$ – Mathieu Guindon Jan 15 '15 at 18:07
  • 2
    \$\begingroup\$ Ouch. Not what I was expecting from you. Kind of goes against everything you have been saying so far. "...then it knows more than it should about the implementation behind the interface it's presented." \$\endgroup\$ – Issa Fram Jan 15 '15 at 18:39
  • 1
    \$\begingroup\$ Actually, the simpler way is to let the IoC container deal with the lifetime of your UoW - that's what IoC containers do! \$\endgroup\$ – Mathieu Guindon Jan 15 '15 at 18:57
  • 2
    \$\begingroup\$ I completely disagree that putting the IDisposable interface on IUnitOfWork is leaky. It simply says that all units of work must be disposed, allowing them to be used in using statements as the OP said. If some implementations have nothing to dispose, it doesn't matter. I think it's far better to comply with the Liskov substitution principle - objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program. \$\endgroup\$ – craftworkgames Jan 16 '15 at 0:30
2
\$\begingroup\$

Disposable pattern is too complicated. It seems you forgot to call Dispose(false) from ~Finalize(). However the implementation can be simplified, since TransactionScopeUnitOfWork doesn't contain unmanaged resources.

public void Dispose()
{
    transactionScope.Dispose();
}

That should be enough.

Further, from the code below it seems that the repository and the unis of the work are independent.

public Test(
    IRepository testRepository,
    IUnitOfWorkFactory unitOfWorkFactory)
{
     this.testRepository = testRepository;
     this.unitOfWorkFactory = unitOfWorkFactory;

     using (IUnitOfWork unitOfWork = this.unitOfWorkFactory.GetUnitOfWork(IsolationLevel.Serializable))
    {
        this.testRepository.Delete(1); // Some valid CRUD
        unitOfWork.Commit();
    }
}

If I understand the purpose of TransactionScope correctly, it's not true. Implementations of these interfaces should be compatible, so if IUnitOfWork is based on TransactionScope, then IRepository should be based on SqlConnection (or OracleConnection, or something other that supports transaction scope).

I'm not sure, but try consider creating the repository and the UoW in single factory:

public interface IDataLayerFactory
{
    IRepository CreateRepository();

    IUnitOfWork CreateUnitOfWord(IsolationLevel isolationLevel);
}

This design makes the relation explicit.

\$\endgroup\$
  • \$\begingroup\$ Not sure what you mean when you state that "you forgot to call Dispose(false) from ~Finalize()." There are no unmanaged resources so I intentionally left the finalizer out. I originally kept the simple this.transactionScope.Dispose() in there but Code Analysis complained about not implementing the Disposable Pattern. I don't know all the details of .NET but if there is a chance that Dispose() gets called twice then I want to make sure I do everything correctly. \$\endgroup\$ – Issa Fram Jan 15 '15 at 19:23
  • 1
    \$\begingroup\$ You are correct that TransactionScope only works for certain classes (provided they have an implementation for link) And yes my repos in this case happen to work with TransactionScope. But yes that is a dependency. I was just doing the correct assignment in the IoC container. If it is incorrect there would be no Unit of Work. But the repo would still perform it's regular tasks. Is this bad you think? My repos are generic and used in my containers so how would that work in a factory? \$\endgroup\$ – Issa Fram Jan 15 '15 at 19:30
  • \$\begingroup\$ @IssaFram As I said, Disposable Pattern is too complicated. :) Just notice, that your Dispose method calls Dispose(true), but Dispose(false) is never called. So you can simplify the code. \$\endgroup\$ – Mark Shevchenko Jan 15 '15 at 19:57
  • \$\begingroup\$ Also notice the disposed field which makes the code different than a simple modification. I prevent the managed Dispose from being called more than once. \$\endgroup\$ – Issa Fram Jan 15 '15 at 20:21
  • \$\begingroup\$ According to the second part: to show the relation between these two entities, it's enough if they both belong the same tier (the same or neighbor namespace). In this case I don't have a clear proposal. \$\endgroup\$ – Mark Shevchenko Jan 15 '15 at 20:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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