Explicitly writing using(...) { ... } or hiding it?

Question

Today I had a discussion with our architect about writing using statements in WorkUnits.

Lets assume we have a PersonWorkUnit with methods:

public class PersonWorkUnit 
{    
    private IContextFactory contextFactory;
    private IRepositoryFactory repositoryFactory;

    public PersonWorkUnit(IContextFactory contextFactory, IRepositoryFactory repositoryFactory)
    {
        this.contextFactory = contextFactory;
        this.repositoryFactory = repositoryFactory;
    }

    public Person Get(int id) 
    {
         using(var context = this.contextFactory.Create<IPersonContext>())
         {
              var personRepo = this.repositoryFactory.Create<IPersonRepository>(context);
              return personRepo.Get(id);
         }
    }
}

What he doesn't like is, that we have to write the using(var context = this.contextFactory.Create<IPersonContext>()) { var personRepo = this.repositoryFactory.Create<IPersonRepository>(context); in every method of the WorkUnit.

My opinion is, that it is good to write (and read) it in every method because you can see explicitly the lifetime of the context and what is done during, or before/after the context. And if you have a usecases where you need multiple repositories.

His opinion is, that it's bad to "duplicate" this code in every method of the class and wants to "hide" it in a method-level-aspect (attribute).

What is your opinion on this topic?

I'm looking forward for insights and reasons/advantages/disadvantages for the solutions.

Thanks in advance

Edit: To answer some questions

• What is a context?
  With context I mean an entity framework DbContext
• Why have a ContextFactory?
  I want to have a factory so I don't have new PersonContext() and I can mock the DbContext in acceptance tests with an InMemoryDbContext
• What is a repository?
  Repositories (and repositories only) are responsible for accessing the database (DbContexts)
• What is a workunit? In WorkUnits we have methods that correspond to use cases. A use case could need multiple (different) repositories. In a WorkUnit method there could be done more than only a call to a repository... the example above is simplified
• Why not use the ContextFactory in repositories? Because some use cases require multiple repositories and I want them to use the same context, so that if something fails, the whole transaction is rolled back

Answer 1

From the way you describe your code, it sounds like PersonWorkUnit has a set of methods that all follow the follow format:

public SomeType Foo(...) 
{
    using(var context = this.contextFactory.Create<IPersonContext>())
    {
        var personRepo = this.repositoryFactory.Create<IPersonRepository>(context);
        // do something with personRepo to create SomeType instance
        // return SomeType
    }
}

If that is the case, then just use delegates to create a single method that does everything bar the commented lines, and supply the latter via a Func<>:

public Person Get(int id) => ProcessPersonRepo(p => p.Get(id));

private T ProcessPersonRepo<T>(Func<IPersonRepository, T> specificBehaviour) 
{
    using(var context = contextFactory.Create<IPersonContext>())
    {
        var personRepo = repositoryFactory.Create<IPersonRepository>(context);
        return specificBehaviour(personRepo);
    }
}

Answer 2

Perhaps Its just your example but this code just looks odd to me. a code smell if you will.

You have four objects which have possible control of the db connection

• The context factory
• The repository
• The work unit
• The context itself

I can see why you might want all these, but its quite a complex setup.

I'm sure your example is deliberately kept short, but it does raise some questions about possible ways to simplify the code.

• If your work unit methods mirror the repositories, why have them at all? Just call the repository.

• Could the context factory not be part of the repository? Perhaps use a connection string constructor.

• What value is added by creating a new repository on each call? Could you not simply inject the repository?

• If your Work Unit methods are bigger than the example, would Transaction Scope be an alternative method of controling the unit?

If you Real Life code justifies the use of all four objects and each has a clear responsibility, then:

Yes, its good practice to dispose of the context after use and...

No, I would say moving the using block to an Aspect Orientated Attribute just throws another complexity into an already complex scenario and obfuscates rather than simplifies the code.

Answer 3

Dealing with context and repository objects are data access level concerns and should be separated from your business objects which is what PersonWorkUnit appears to be. You should have a data access class that only handles using the context and repository. this would make your business code simpler by only having something like dataGetter.GetPerson(Id); in your get method. If you know the person you need at creation of the PersonWorkUnit you could even move this call to the constructor which will simplify the code that calls PersonWorkUnit as well. All the ugly boiler plate code is then in dataGetter where it can be ignored for the most part.

The goal of this setup is to maintain separation of concerns so your business logic doesn't have to deal with infrastructure logic, and it also allows you to follow the open closed principle. with your example design PersonWorkUnit must change if data access rules change and if business rules about person change. By having a dataGetter class that returns a person, the only reason for PersonWorkUnit to change is because the business logic changes.