3

I have some questions about using the using statement. I understand what it does (If I'm correct, it disposes all open connections and such), but I'm unsure how I should use it correctly.

The project I'm working in doesn't contain any repositories, which you don't need for Entity Framework.

So basically, I'm getting a list of Guids as parameter in my method. These ids are for restaurants, and I want to retrieve all reviews that have been given on these restaurants.

So currently, I'm retrieving the list like so:

public void DoSomething(List<Guid> restaurantIds)
{
    List<Review> reviews;
    using (var db = new Context())
    {
        reviews = db.Reviews.Where(x => restaurantIds.Contains(x.RestaurantId)).ToList();
    }
    //More stuff here
}

Is this a common bad practice to declare the list outside of the using statement? I thought of a few alternatives, but I'm (again) unsure what would be better.

  1. Create a seperate method in the same class which does exactly that and returns the list. This way in my DoSomething method, I can just use it like this: List<Review> reviews = GetReviewsFromRestaurants(restaurantIds);
  2. I have to create the context first, and then use the LINQ statement without the using block. I have to call .Dispose() when I'm done with it though.

Is there a problem when I use the using statement like in my example? If so, are the alternatives better? And if that's not the case, could you give me an example on how I should retrieve this list?

Improve this question
8
  • Can you elaborate what you call bad practice? Declaration of variable outside the using statement? Commented Jan 5, 2017 at 9:37
  • Yes, that. I'll add it to my post. Commented Jan 5, 2017 at 9:37
  • 1
    This is not a bad practice per se, but there can be a problem in this case if you try to lazy load navigation properties after the context has been disposed. Commented Jan 5, 2017 at 9:41
  • As long as you call .ToList(), this shouldn't be a problem though, right? I'm aware of lazy loading, so I'm sure to call .ToList() every time I retrieve something from my database. Commented Jan 5, 2017 at 9:42
  • 1
    @RandomStranger - you mention multiple hits in the same method, would it be a problem to extend my using block to cover the entire method - That should be fine || As long as you call .ToList(), this shouldn't be a problem though, right? - that's not right. Say your Review entity has SomeRelationalEntity which is not eager loaded and when you try to access SomeRelationalEntity from the list you got (even after doing a ToList()), EF will hit the database and fetches it for you (lazy loading) if that happens in the same context Commented Jan 5, 2017 at 9:53

3 Answers 3

Reset to default
3

Here in this case braces defines their own scope. The variables which you will declare outside the scope of braces will be visible inside braces and its okay.

Its actually the shorthand for try catch block. 

List<Review> reviews;
var db = new Context();
try
{
   reviews = db.Reviews.Where(x => restaurantIds.Contains(x.RestaurantId)).ToList();
}
finally
{
  db.Dispose();
}

And your snippet is much more concise than this. Compiler will always call .Dispose on the "used" object.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

About the reviews variable, I do not think it is bad that you declare it outside of using block.

1. Separate method is recommended. Actually you can still use a repository class (RestaurantRepository) that will handle all these basic operations like: get all restaurants based on a single or multiple identifier, create a new restaurant, change some data for a restaurant.

This ensures that you separate your business logic from basic operations.

2. Disposable context vs. explicit Dispose(). Disposable context is clearly better since it you make sure that Dispose() is called even if your code fails.

Bigger picture - Entity framework and context dispose

This is already discussed here. Also, this article shows that dispose is not so necessary as it seems.

Personally, I have been Unit of Work pattern to allow multiple changes (on various repositories / entities) and did not get into trouble for not disposing the context.

Improve this answer

5 Comments

Yeah I agree with the repository. Problem is, the team I'm in does not and as an intern, my decisions don't have a lot of weight.
It's not as much for repository pattern, as having basic operations separated to have clear code and reusability. DoSomething performs both filtering and "something". If some other method wants to do something else on some filtered list of Reviews, it will have to filter on its own.
Ah I see, so I don't particularly need a repository, but creating a separate method in this class would be a good practice?
Yes, another method would be fine.
Somehow out of topic. As this answer seems to be the most complete one. I'll point something out here. I saw this type of pattern of using 'using' and dispose the connection right after you finished the db query when companies used Azure. As some time ago (i don't know exactly if it's the case right now) the open connection influenced the price that your company paid.
0

There usually is no one-size-fits-all answer to "what is a bad practice" and your question is no exception. While often thare are clearly bad prectices and sometimes clearly good practices, there is a lot of factors that influence your decision (such as your requirements, the rules of your organization or the experience of your team).

You can keep a context in entity framework for longer than just a moment (although keeping it open for very long might also not be a good idea), as long as you are sure you dispose it at the end. But there is also nothing wrong in that way you apply using to your context. In web applications contexts are often used for a very short time and low-level connection pooling usally keeps the solution still performant.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.