Repository pattern best practices using EF 6

I have been Googling for a few days and am trying to find the best practices when it comes to using the Repository pattern. But what I found that there are no standards and everyone claims that their approach is the best approach. However, I have come up with this design based on what I have read online.

It it good practice to pass DataContext? Please feel free to correct me and explain.

I'm using MVC 5 with EF 6.

public interface IRepository where TEntity  : class
 {
        IEnumerable GetUsers();
        IQueryable SearchFor(Expression> predicate);
        TEntity GetById(int id);
        void Save(TEntity model);
        void Delete(int id);
 }

public class HostRepository : IRepository
{
    private EntityDataModelContext db = new EntityDataModelContext();

    //
    public IEnumerable GetUsers()
    {
        var userList = from u in db.Hosts select u;
        var users = new List();

        if (userList.Any())
        {
            foreach (var user in userList)
            {
                users.Add(new HostViewModel() { Id = user.Id,  FirstName = user.FirstName });
            }

        }
        return users;
    }
    //implement rest here......
 }

//I use an IoC framework to inject the dependencies

public class HostController : Controller
{
    public IRepository hostRepository { get; set; }

    public HostController(IRepository hostRepo)
    {
        hostRepository = hostRepo;
    } 

    public ActionResult Index()
    {
        var users = hostRepository.GetUsers();
        return View(users);
    }
}

The Interface

-
The idea of a generic repository interface is that it should be applicable to all entity types. So GetUsers shouldn't be there, because it's only relevant for a particular type of entity. If you want members specific to an entity type, you should have a second interface which extends the generic one:

IUserRepository : IRepository
{
    IEnumerable GetUsers();
}

-
One of the debates you may well have seen is whether it's good practice to have IQueryable members, or to take Expression parameters.

The pro is that it's much more flexible. Rather than having to have, for example, FilterByStartDate(DateTime date), FilterByEndDate(DateTime date), FilterByUserGroup(int groupId), etc., I can just have FilterBy(Predicate condition). Likewise, by exposing IQueryable, you can call additional LINQ methods and it will all get translated into SQL, which can reduce the amount of work your database has to do, and the amount of data you have to transfer.

The con is that what you have is a leaky abstraction. It looks like I should be able to feed in any reasonable predicate, but in fact, many of them will throw exceptions at runtime because EF doesn't know how to translate them into SQL. So now the details of how exactly EF- which is supposed to be an abstraction over your database- can turn LINQ queries into SQL leak out of your repository, and every consumer of the repository has to worry about them.

On balance, my suggestion would be: at first, do NOT expose IQueryable or allow arbitrary Expressions to be passed in. Add specific methods for specific queries. If you ultimately find this isn't adequate, you can refactor by adding methods in.

-
Save is a confusing method. Does it mean insert or update? Generally with Entity Framework, you want an Add(TEntity entity) method and a Save() method. Add should be what inserts a record, and Save should commit any updates you've made to existing items. So, as a toy example, I could do:

var user = userRepository.GetById(userId);
DoThing(user);
user.ThingLastDoneTime = DateTime.Now;
userRepository.Save();

Note that I don't have to pass the user to Save, because it's actually just calling the DbContext's Save() method.

HostViewModel

This is where I start to get a bit more confused. Why is something called ViewModel being treated as an entity, or stored in a database?

Without knowing more it's hard to comment on what's going on here exactly, but EF is supposed to let you handle Entities- these are essentially domain objects. So a User is a good candidate for an entity. A Host might be, depending on what "host" actually means in this context. But a ViewModel? That seems like extreme mixing of concerns between your UI and your core domain.

Or, possibly what's going on is that your actual entity is a Host, and the repository is not only doing its usual repository business, but also in charge of mapping from an entity to a view model. This is also a problematic mixing of concerns. You may, for example, have a view model which does not have sufficient information to create a new entity from. So how would you handle the Add method for that type? Transformation between entities and view models is a UI concern and should be kept away from the repository pattern, which is a data access/persistence pattern.

HostRepository

As already mentioned, it's very weird that GetUsers should be returning a collection of HostViewModels.

The bulk of that method can be simplified with LINQ, too:

return db.Hosts.Select(user => new HostViewModel() { Id = user.Id,  FirstName = user.FirstName });

The Controller

• I assume your comment about IoC containers is for us and not really in the code. If it's in the code, it shouldn't be!

• hostRepository should also be private. Other classes don't need to get it, and shouldn't be allowed to set it except through the constructor.

• You've shortened hostRepository to hostRepo in the constructor. Generally you shouldn't shorten variable names, especially if- as in this case- they're going to be exposed publically to consumers. To deal with class-level vs method level variables, all of the following are popular:

• For public class members, use PascalCase (first letter capital) rather than camelCase

• For private class fields and properties use _camelCase, with a leading underscore.

• As an alternative to the previous, for private class fields and properties, use camelCase. Then inside methods, prefix them with this.

StackExchange Code Review Q#57401, answer score: 7