How to implement DRY principle when using 'using' keyword?

Question

Consider these methods:

public List<Employee> GetAllEmployees()
{
    using (Entities entities = new Entities())
    {
        return entities.Employees.ToList();
    }
}

public List<Job> GetAllJobs()
{
    using (Entities entities = new Entities())
    {
        return entities.Jobs.ToList();
    }
}

public List<Task> GetAllTasksOfTheJob(Job job)
{
    using (Entities entities = new Entities())
    {
        return entities.Tasks.Where(t => t.JobId == job.Id).ToList();
    }
}

Using block is the same and has been repeated 3 times here (of course, more than 100 times in the real application). How is it possible to implement DRY (Don't Repeat Yourself) principal for using block? Is it considered a breach of DRY principal at all?

Update: I'm not talking about what has been implemented inside the using block. What I actually mean here, is the using (Entities entities = new Entities()). This line is repeated 100 times or more.

Answer 1

One idea would be to wrap it with a function that takes a Func.

Something like this

public K UsingT<T,K>(Func<T,K> f) where T:IDisposable,new()
{
    using (T t = new T())
    {
        return f(t);
    }
}

Then your above code becomes

public List<Employee> GetAllEmployees()
{
    return UsingT<Entities,List<Employee>>(e=>e.Employees.ToList());
}

public List<Job> GetAllJobs()
{
    return UsingT<Entities,List<Job>>(e=>e.Jobs.ToList());
}

public List<Task> GetAllTasksOfTheJob(Job job)
{
    return UsingT<Entities,List<Task>>(e=>e.Tasks.Where(t => t.JobId == job.Id).ToList());
}

I made Entities a type param too, because I'm assuming you have more than one type you're doing this with. If you're not, you could remove it and just use the type param for the return type.

To be honest though this sort of code doesn't help readability at all. In my experience the more Jr co-workers have a really tough time with it as well.

Update Some additional variations on helpers you might consider

//forget the Entities type param
public T UsingEntities<T>(Func<Entities,T> f)
{
    using (Entities e = new Entities())
    {
        return f(e);
    }
}
//forget the Entities type param, and return an IList
public IList<T> ListFromEntities<T>(Func<Entities,IEnumerable<T>> f)
{
    using (Entities e = new Entities())
    {
        return f(e).ToList();
    }
}
//doing the .ToList() forces the results to enumerate before `e` gets disposed.

Answer 2

To me this would be like worrying about foreach-ing over the same collection multiple times: it's just something that you need to do. Any attempt to abstract it further would make the code much less readable.

Answer 3

It sounds like you are confusing the "Once and Only Once" principle with the DRY principle. The DRY principle states:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

However the Once and Only Once principle is slightly different.

[The DRY] principle is similar to OnceAndOnlyOnce, but with a different objective. With OnceAndOnlyOnce, you are encouraged to refactor to eliminate duplicated code and functionality. With DRY, you try to identify the single, definitive source of every piece of knowledge used in your system, and then use that source to generate applicable instances of that knowledge (code, documentation, tests, etc).

The DRY principle is usually used in context of actual logic, not so much redundant using statements:

Keeping the structure of a program DRY is harder, and lower value. It's the business rules, the if statements, the math formulas, and the metadata that should appear in only one place. The WET stuff - HTML pages, redundant test data, commas and {} delimiters - are all easy to ignore, so DRYing them is less important.

Source

Answer 4

I fail to see the use of using here:

How about:

public List<Employee> GetAllEmployees() {
    return (new Entities()).Employees.ToList();
}
public List<Job> GetAllJobs() {
    return (new Entities()).Jobs.ToList();
}
public List<Task> GetAllTasksOfTheJob(Job job) {
    return (new Entities()).Tasks.Where(t => t.JobId == job.Id).ToList();
}

Or even better, as I don't think you need to create a new object every time.

private Entities entities = new Entities();//not sure C# allows for that kind of initialization, but you can do it in the constructor if needed

public List<Employee> GetAllEmployees() {
    return entities.Employees.ToList();
}
public List<Job> GetAllJobs() {
    return entities.Jobs.ToList();
}
public List<Task> GetAllTasksOfTheJob(Job job) {
    return entities.Tasks.Where(t => t.JobId == job.Id).ToList();
}

As for breaching DRY: DRY doesn't apply on this level. In fact no principle really does, except that of readability. Trying to apply DRY at that level is really just architectural micro-optimization, which like all micro-optimization is just bike-shedding and doesn't get any problems solved, but even risks to introduce new ones.
From my own experience, I know that if you try to reduce code redundancy at that level, you create a negative impact on code quality, by obfuscating what was really clear and simple.

Edit:
Ok. So the problem is not actually the using statement, the problem is the dependency on the object you create every time. I would suggest injecting a constructor:

private delegate Entities query();//this should be injected from the outside (upon construction for example)
public List<Employee> GetAllEmployees() {
    using (var entities = query()) {//AFAIK C# can infer the type here
        return entities.Employees.ToList();
    }
}
//... and so on

Answer 5

Not only using is duplicate code (by the way it is duplicate code and actually compares to a try..catch..finally statement) but the toList also. I would refactor your code like this:

 public List<T> GetAll(Func<Entities, IEnumerable<T>> getter) {
    using (Entities entities = new Entities())
    {
        return getter().ToList();
    }
 }

public List<Employee> GetAllEmployees()
{
    return GetAll(e => e.Employees);
}

public List<Job> GetAllJobs()
{
    return GetAll(e => e.Jobs);
}

public List<Task> GetAllTasksOfTheJob(Job job)
{
    return GetAll(e => e.Tasks.Where(t => t.JobId == job.Id));
}

Answer 6

Since there is no business logic of any sort here except for the last one. Its not really DRY, in my view.

The last one doesnt have any DRY in the using block but I guess the where clause should change where ever it used.

This is a typical job for code generators. Write and cover code generator and let it generate for each type.

Answer 7

Since you're creating and destroying the same disposable object over and over, your class is itself a good candidate for implementing the IDisposable pattern.

class ThisClass : IDisposable
{
    protected virtual Entities Context { get; set; }

    protected virtual void Dispose( bool disposing )
    {
        if ( disposing && Context != null )
            Context.Dispose();
    }

    public void Dispose()
    {
        Dispose( true );
    }

    public ThisClass()
    {
        Context = new Entities();
    }

    public List<Employee> GetAllEmployees()
    {
        return Context.Employees.ToList();
    }

    public List<Job> GetAllJobs()
    {
        return Context.Jobs.ToList();
    }

    public List<Task> GetAllTasksOfTheJob(Job job)
    {
        return Context.Tasks.Where(t => t.JobId == job.Id).ToList();
    }
}

This leaves you only needing the "using" when creating an instance of your class. If you don't want the class to be responsible for disposing the objects, then you could make the methods accept the dependency as an argument:

public static List<Employee> GetAllEmployees( Entities entities )
{
    return entities.Employees.ToList();
}

public static List<Job> GetAllJobs( Entities entities )
{
    return entities.Jobs.ToList();
}

public static List<Task> GetAllTasksOfTheJob( Entities entities, Job job )
{
    return entities.Tasks.Where(t => t.JobId == job.Id).ToList();
}

Answer 8

My favorite bit of uncomprendable magic!

public class Blah
{
  IEnumerable<T> Wrap(Func<Entities, IEnumerable<T>> act)
  {
    using(var entities = new Entities()) { return act(entities); }
  }

  public List<Employee> GetAllEmployees()
  {
    return Wrap(e => e.Employees.ToList());
  }

  public List<Job> GetAllJobs()
  {
    return Wrap(e => e.Jobs.ToList());
  }

  public List<Task> GetAllTasksOfTheJob(Job job)
  {
    return Wrap(e => e.Tasks.Where(x ....).ToList());
  }
}

Wrap exists only to abstract that out or whatever magic you need. I'm not sure I would recommend this all the time but it's possible to use. The "better" idea would be to use a DI container, like StructureMap, and just scope the Entities class to the request context, inject it into the controller, and then let it take care of the lifecycle without your controller needing to.