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<User>
{
IEnumerable<User> 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<T>(Predicate<T> 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 Expression
s 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 HostViewModel
s.
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.