Basic Entity Framework Repository implementation

Question

Here is simple repository pattern that I use to access data in my database. I would like some advice on how to improve this code. Commenting on my code quality is also welcome too!

public static class ProdRepository
{
    private static Entities context = null;

    public static Entities Context
    {
        get
        {
            if (context == null)
            {
                return new Entities();
            }
            else
                return context;

        }
    }

    /// <summary>
    /// Getting Outbound list in selected period of dates
    /// </summary>
    /// <param name="dateFrom">begin date</param>
    /// <param name="dateTo">end date</param>
    /// <returns>Outbound List</returns>
    public static List<OutboundModel> GetOutboundsByDate(DateTime dateFrom, DateTime dateTo)
    {
        var q = from o in Context.wms_Outbound
                where o.CreateDT <= dateTo && o.CreateDT >= dateFrom
                select new OutboundModel
                {
                    Id = o.Id,
                    Code = o.Code,
                    CreationDate = o.CreateDT,
                    RootQuantity = o.RootQuantity
                };

        return q.ToList();

    }

}

Answer 1

Very nice and comprehensive. Your names are clear and short, your code is obvious in it's intent. I just see two things that startle me a bit:

if (context == null)
{
   return new Entities();
}
else
   return context;

what you do here is ... amusing. This code doesn't do what you think it does ;)

Both blocks are single line-statements, yet you put the first one in brackets and the second one you don't. This is inconsistent and prone to bugs. Let's say you want to log when someone accessed the context without a new instance created.

The "logical", but wrong code would be:

//if bla bla
else
    Logger.log("Context accessed.", Log.DEBUG);
    return context;

While this would work as planned if your if did what you seem to think it does, the actual flow of code is incorrect.

That whole thing should be rewritten as:

if (context == null)
{
    context = new Entities();
}
return context;

This does what you seem to have had in mind. In your code, you never assign to context, which remains null until all eternity. With each call to the getter you just return a new instance of Entities().

And the next thing that startles me is this:

public static List<OutboundModel> GetOutboundsByDate(DateTime dateFrom, DateTime dateTo)

in combination with the fact that your output is not sorted. Why? Using the Type List , especially in combination with the name, IMO implies a sorting of the data set. In this case your sorting is the Id, but for GetOutboundsByDate, I expect a data set that is ordered by date (and not by Id)

Instead I'd use IEnumerable<T>.

Answer 2

Naming wise, I wouldn't shorten from, what I assume is Product, to Prod, make your class names and variables unambiguous.

I'd perhaps call your Entities() EF context something more descriptive too. Whilst it may not seem necessary now, I've worked on 4-5 projects which have ended up with more than one context for a number of reasons, having one called Entities can quite easily end up being unclear over time.

Structure wise, you probably won't want your repository as a static class, as you'll likely want to inject it into other classes as a dependency (and if you aren't thinking along those lines you probably should be). This will allow you to substitute your EF repository for a testing repository or stub/mock if/when you want to unit test.

Following this through, I'd expect to see your Repository implement IProductRepository.

Answer 3

Two questions.

1. Do you really need to specifically return a List<OutboundModel> or would an IList<OutboundModel> or even IEnumerable<OutboundModel> do?
2. Since context starts out as null and is never changed, why does it even exist? The compiler will optimize it away and change the property to just return new Entities();. Is that your intention?

Answer 4

I'm the kind of person who doesn't like to see a repository not interfaced! You should create an IProductRepository, so you can change the implementation for a mock maybe (if you do unit testing) or if one day you wake up telling yourself you would now use NHibernate, which would require to change your repository implementation, but not the place it is used.The point of the interface would be to hide the fact that you use EF.

Your interface method for GetOutboundsByDate should return an IEnumerable<OutboundModel> because the repository returns you a view of your datastore. Returning a List isn't a good practice since it exposes internal implementation of your object. Plus, will you ever want to Add to this List<>? I doubt..

Also, has some others have pointed, your Context property doesn't do what you want, you should do :

return context  ?? (context = new Entities()); 

which would return your context if it is not null or assign a new Entities instance to your context if it is null (the ?? operator) and return context. What you actually do is create a new Èntites everytime you access your property (see that context is never assigned!)

Also, I am not sure your Context property should be public since you want to have control over your DbContext, I believe you should make it private and create methods that would access it for the clients of your Repository (am I clear? If not, tell it in the comments!)