Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
1  
is this C# ? The answer to your question could be language dependant –  David Aug 26 '11 at 12:38
    
Yeah @David, sorry that I didn't mention my language. How it can affect the answer? –  Saeed Neamati Aug 26 '11 at 12:41
    
some languages can have particular syntax that can help you factor a bit of your code. I don't know C#, but in Ruby, I think you could use blocks to factor the using part. –  David Aug 26 '11 at 12:50
    
The using statement actually provides C# language support to apply the DRY principle to help avoid repetitive coding while managing resource disposal with the Dispose design pattern. That doesn't mean we can't find ways to make things DRYer! Personally, I think of DRY as a recursive process. –  John Tobler Aug 26 '11 at 17:10
add comment

8 Answers

up vote 21 down vote accepted

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.
share|improve this answer
1  
+1 Nice solution, although it doesn't address the actual problem (not included in the original question), i.e. the many occurences of Entities. –  back2dos Aug 26 '11 at 13:18
    
@Doc Brown: I think I beat you to it with the function name. I left the IEnumerable out of the function in case there were any non-IEnumerable properties of T that the caller wanted returned, but you're right, it would clean it up a bit. Perhaps having a helper for both Single and IEnumerable results would be good. That said, I still think it slows down recognition of what the code does, especially for someone who is not accustomed to using lots of generics and lambdas (eg your coworkers who are NOT on SO :) ) –  Brook Aug 26 '11 at 13:18
    
+1 I think this approach is fine. And readability can be improved. For example, put the "ToList" into WithEntities, use Func<T,IEnumerable<K>> instead of Func<T,K>, and give "WithEntities" a better name (like SelectEntities). And I don't think "Entities" needs to be a generic parameter here. –  Doc Brown Aug 26 '11 at 13:20
1  
To make this work, the constraints would need to be where T : IDisposable, new(), as using requires IDisposable in order to work. –  Anthony Pegram Aug 26 '11 at 13:43
1  
@Anthony Pegram: Fixed, thanks! (that's what I get for coding C# in a browser window) –  Brook Aug 26 '11 at 13:48
show 4 more comments

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.

share|improve this answer
    
Great analogy @Ben. +1 –  Saeed Neamati Aug 26 '11 at 14:51
2  
I don't agree, sorry. Read the comments of the OP about the transaction scope and think about what you have to do when you have written 500 times those kind of code, and then notice that you will have to change the same thing 500 times. This kind of code repeating may be ok only if you have <10 of those functions. –  Doc Brown Aug 26 '11 at 16:39
    
Oh, and not to forget, if you for-each the same collection more than 10 times in a very similar manner, with similar-looking code inside of your for-each, you should definitly think about some generalization. –  Doc Brown Aug 26 '11 at 16:46
    
To me it just looks like you are making a three liner into a oneliner... you're still repeating yourself. –  FRoZeN Nov 8 '12 at 15:22
    
It's context dependant, if the foreach is over a very large collection or the logic within the foreach loop is time consuming for example. A motto I have come to adopt: Don't obsess but always be mindful of your approach –  CodeBlend Jan 29 '13 at 13:58
add comment

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
share|improve this answer
2  
But @back2dos, there are many places where we use using (CustomTransaction transaction = new CustomTransaction()) code block in our code to define the scope of a a transaction. That can't be bundled into a single object and in every place you want to use a transaction, you should write a block. Now what if you want to change the type of that transaction from CustomTransaction to BuiltInTransaction in more than 500 methods? This seems to me to be a repeating task and a breach example of DRY principal. –  Saeed Neamati Aug 26 '11 at 12:29
3  
Having "using" here closes the data context so lazy loading is not possible outside of these methods. –  WeekendWarrior Aug 26 '11 at 13:00
    
@Saeed: That's when you look into dependency injection. But that seems to be to be quite different from the case as stated in the question. –  Michael Kjörling Aug 26 '11 at 13:02
    
@Saeed: Post updated. –  back2dos Aug 26 '11 at 13:15
    
@WeekendWarrior Is using (in this context) still a more unknown "convenient syntax"? Why it's so nice to use =) –  CodeBlend Jan 29 '13 at 14:03
add comment

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

share|improve this answer
add comment

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));
}
share|improve this answer
add comment

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.

share|improve this answer
    
No @arunmur. There was a misunderstanding here. By DRY, I meant using (Entities entities = new Entities()) block. I mean, this line of code is repeated 100 times and is getting repeated more and more. –  Saeed Neamati Aug 26 '11 at 12:23
    
DRY primarily comes from the fact that you need to write test cases that many times and a bug at one place means you have to fix it at 100 places. The using(Entities ...) is too simple a code to break. It does not need to be split or put in another class. If you still insist on simplifying it. I would suggest a Yeild call back function from ruby. –  arunmur Aug 26 '11 at 13:17
1  
@arnumur - using is not always too simple to break. I often have a good bit of logic that determines which options to use on the data context. It is very possible that the wrong connection string could be passed in. –  Morgan Herlocker Aug 26 '11 at 13:28
1  
@ironcode - In those situations it makes sense to push it off to a function. But in these examples its pretty tough to break. Even if it does break the fix is often in the definition of the Entities class itself, which should be covered with a separate test case. –  arunmur Aug 26 '11 at 14:47
    
+1 @arunmur - I agree. I usually do that myself. I write tests for that function, but it does seem a bit over the top to write a test for your using statement. –  Morgan Herlocker Aug 26 '11 at 16:49
add comment

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();
}
share|improve this answer
add comment

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.

share|improve this answer
    
Your type paramaters for Func<IEnumerable<T>,Entities> are in the wrong order. (see my answer which has basically the same thing) –  Brook Aug 26 '11 at 17:33
    
Well, easy enough to correct. I never remember what the right order is. I use Funcs enough I should. –  Travis Aug 27 '11 at 1:53
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.