2

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
  • 2
    The code does look odd. with the multiple layers of wrapping and only doing one thing in the method. – Ewan Sep 27 '17 at 9:49
  • Do you handle transactions ? If so the pattern you're using seems unlikely able to do so (and if it does, it really don't like it, so that could be consider as a code smell) – Walfrat Sep 27 '17 at 10:33
  • @Walfrat Entity Framework handles transactions itself... if you open a new DbContext and call .SaveChanges() on it, it creates a transaction itself – xeraphim Sep 27 '17 at 10:42
  • @Walfrat what exactly do you consider a code smell? And what do you propose to change? – xeraphim Sep 27 '17 at 10:42
  • 1
    I don't really know entity framework, so what I mean by handling transaction, is, if I call two of your methods that performs writes, will they be part of the same transaction ? If so it could be usefull to add it in your question, because without knowing it that code seems to handle every call separately, or is that it's on a purpose ? A workUnit represent a transaction independant frm another work unit ? – Walfrat Sep 27 '17 at 10:47
6

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);
    }
}
  • 4
    You may also want an Action<IPersonRepository> overload, if T would otherwise be void – Caleth Sep 27 '17 at 12:38
  • This is the recommended way whenever you wish to wrap an unknown function with known code. – Frank Hileman Sep 27 '17 at 17:56
1

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.

  • Hi @Ewan thanks for your answer, I've added some insights to the original question to clarify things – xeraphim Sep 27 '17 at 10:22
  • no prob. Personally I would stick with one repo per db, have it manage the context and use transaction scope. But I know other people use your pattern, I don't see a problem with it – Ewan Sep 27 '17 at 10:27
-2

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.

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.