# Entity Framework, code-first repository pattern review. Where to validate?

I've been iterating on my repository pattern implementation over the course of the past 4-5 months. In my new projects I choose to use this pattern and I try to improve upon what I learned in previous projects. I'm pretty pleased with the state of my current implementation. I feel like I could improve upon my validation logic somehow, though.

First and foremost, my code is on GitHub in its entirety: https://github.com/ryancole/Warcraft

### Description of my implementation

I've gone with a layered approach consisting of my POCO entity classes, the repositories and finally services on top of the repositories. I have a service layer because I've chosen to use a generic repository class. My repository class is basic and provides the core CRUD operations. It's also purposefully leaky, because it's intended to sit behind my service layer - so, my repository class returns IQueryable. My service layer returns ICollection, on the other hand.

public interface IRepository<T> where T : class
{
#region Properties

/// <summary>
/// Retrieve all entities
/// </summary>
IQueryable<T> All { get; }

#endregion

#region Methods

/// <summary>
/// Count entities matching a predicate
/// </summary>
int Count(Expression<Func<T, bool>> predicate);

/// <summary>
/// Insert an entity
/// </summary>
T Insert(T entity);

/// <summary>
/// Retrieve entities matching a predicate
/// </summary>
IQueryable<T> Where(Expression<Func<T, bool>> predicate);

#endregion
}


I have a unit of work class, which contains my DbContext instance and provides access to my various services. The unit of work class also exposes a SaveChanges method. The intention is that users of my libraries will just pass this unit of work class through to their MVC controllers, and therefor do not have to deal with any of the underlying contexts, repositories, etc - only the services with their explicitly crafted methods for specific tasks.

public interface ISession : IDisposable
{
#region Properties

IAccountService Accounts { get; }
ICharacterService Characters { get; }

#endregion

#region Methods

bool SaveChanges();

#endregion
}

public interface ICharacterService
{
#region Methods

/// <summary>
/// Insert a new Character
/// </summary>
Character Insert(Character character);

/// <summary>
/// Retrieve a Character with the specified Name
/// </summary>
Character GetByName(string name);

/// <summary>
/// Retrieve all Characters
/// </summary>
ICollection<Character> GetAll();

/// <summary>
/// Retrieve all Characters for the given Account
/// </summary>
ICollection<Character> GetByAccount(Account account);

#endregion
}


The only thing that I think if slightly leaky is updating an existing entity. Updating an entity is just done by relying on the DbContext to track the changes, and then calling the unit of work's SaveChanges method. So, updating entities is not funneled through any service method. And this creates my current situation and question about my implementation.

### My question

Where can I perform validation that works for both inserts and updates, with minimal repeating of myself?

My original intention and plan was to perform all validation inside of my service layer. So, when adding a new entity you'd call the Insert method and I'd just validate in there. But, with how I do updating of existing entities, there is no Update method, and therefor no place to put that validation in my service layer.

So, what I'm doing now instead is just putting basic validation inside of the POCO entity classes themselves, using DataAnnotations. Any validation that requires data from the database is done in the DbContext override method ValidateEntity. This method works nice, because of how well it integrates with EF for inserts and updates. But, this method kind of sucks because validation is split up into 2 different places and neither are my desired location which would be the service layer!

What do you think of my implementation of the repository pattern? Any improvements I could make?

Do you know of any better way to perform validation that works for both inserts and updates? Any way using dep injection or anything?

• Welcome to CR! Awesome first post, keep 'em coming! :) – Mathieu Guindon Jan 24 '14 at 1:45
• Is Character one of your POCO classes? – Mathieu Guindon Jan 24 '14 at 2:18
• Yea, it is. Just a plain class with some data annotations and IValidatableObject logic. – Ryan Jan 24 '14 at 2:21

## 1 Answer

The only thing that I think if slightly leaky is updating an existing entity.

It isn't the only leak you have:

public interface ISession : IDisposable


This is a leaky abstraction. You have a specific implementation in mind - one that implements IDisposable, and you're leaking this specific implementation into the ISession interface.

That said, I don't really see what the abstract repository earns you. I've seen people wrap EF with a repository/unit-of-work so that they could "swap the ORM" as needed (if that's ever needed), and I can understand why someone really willing to decouple their service layer from the underlying data access technology would want to do that.

But you're purposely leaking IQueryable<T> into your service layer, which defeats the purpose of the repository abstraction.

Why bother? The added complexity doesn't seem to simplify extending the application. I think your service layer can afford to play with EF directly - DbContext is a repository and a unit-of-work. No need to look further.

What you've identified as a leak with the updating of entities, stems from the fact that your service layer is passing entities to your Web/UI layer - you need a seam here: entities belong with EF and the data-related stuff, not in your UI.

I have to admit I don't do much Web development, but in WPF I'd make that service layer work with ViewModel objects, and be able to "translate" a ViewModel into one or more entities. This "seam" completely decouples your front-end from your back-end. Yes, often the ViewModel class will look very similar to the entity class it's wrapping (see this question for a WPF example), but it will only expose what the UI is interested in, so you can keep fields like Id, DateInserted and DateUpdated in the service and data layers and not be bothered with them in the UI layer. I don't think that concern is WPF-specific.

With the service layer performing the "translation" between ViewModel and entity types, you have a perfectly logical spot to perform your validations in, exactly where you wanted to do them in the first place.

• Hmm. I really like the sound of your answer. I'm currently trying to absorb what you said and think about how it'd look. So, if I scrap the generic repository class, which is indeed pointless as far as I'm concerned, and make my services translate between ViewModel and Entity ... I'm just trying to picture what these ViewModel classes would look like. It sounds like I would need a lot of them, one for every possible View. Also, my service methods would need to be very specific as well, one for each View also, basically. – Ryan Jan 24 '14 at 3:09
• Thanks! I might be completely wrong though. I hope to see another review from a C# programmer that's more into ASP.NET/MVC; my WPF/Windows background might be making me say weird things. But I like that you like the sound of it! :) – Mathieu Guindon Jan 24 '14 at 3:17
• Well, I feel like the general concept is the same. If you look at my code on Github, you'll see that I do use seperate view model classes in the web project, except that they directly use the actual entity classes. I'm still trying to properly understand how to break up dependency though. I'm looking at the other question you linked, and I'm trying to let it sink in. – Ryan Jan 24 '14 at 3:21
• Does it make sense though that my service layer will basically have a method specifically for handling every view? It seems very specific, and not useful for other applications other than my ASP.NET application. Unless I'm thinking too specific. – Ryan Jan 24 '14 at 3:24
• I'm not sure if the service layer should be doing the mapping. That sounds like you are mixing it's responsibilities to me. I would consider having a mapping layer and the service separate. – dreza Jan 24 '14 at 18:59