20

I'd second TopinFrassi's answer, that this is a more or less sensible approach, and the implementation is excellent. However, there are some potential issues, which I'll expand from craftworkgames' comments and your response. Because I can't really find anything that needs criticism in how it's written, this will be entirely design focussed. There are two ...


14

Leaky Abstractions public interface IUnitOfWork : IDisposable { void Commit(); } I like that there's only a Commit method here - it makes the IUnitOfWork interface very well segregated/focused. However the interface specifying that all implementations must also implement IDisposable, is a leaky abstraction - you have a specific implementation in mind, ...


12

I get what you mean by "reversed": As opposed to: Makes sense, at least to me - the way I see UoW/Repository pattern (everybody seems to have their own take at this one, eh?), Entity Framework's DbContext is a unit-of-work, and an IDbSet<TEntity> is a repository. Hence, I tend to agree with having unit-of-work depend on repositories and not the ...


11

I leaked Entity Framework into my domain services, because [...] Fail. The only reason you would ever want to wrap EF with your own repository+unit-of-work implementation, would be to make an interface between EF and your code, possibly to enable swapping EF for something else at one point or another. By leaking it, you defeat the entire purpose of the ...


10

Let me know if I went wrong somewhere. This is the first time I set up an application with a proper architecture. I'm afraid there is no such thing as general "proper" architecture. Relevant architecture is the one that enables/assists developers in implementing new functionality or adjusting your solution to new requirements. In your implementation I don'...


9

I think your abstract class should "implement" the IRepository interface, because otherwise you will always have to put both the interface and the base class in your inheritance list. Also, I think you should name your Repository -> SqlRepository, since all the code in the base class is coupled to Sql. As @Heslacher pointed out, you should inject your ...


8

The NewItem implementation in the CustomerGroupsRepository has something weird going on: Set NewItem = result.Mock(model, items) Why add a Mock method to the SqlResultRow type? The SqlResultRow worked fine before, it shouldn't have been modified to accomodate the IRepository implementations. In fact, the NewItem implementation could be written like this: ...


8

I think this is pretty much done. You have great naming. No glaring bugs as far as I can tell. It's clear and concise. Very OOP, which is impressive given the language. Even the high level design seems pretty darn tight. (I know very little about dependency injection though. I could have missed something that would be obvious to someone else.) I just can't ...


8

I can understand why you want to remove the clutter from your code - but sharing the instance of MySqlConnection isn't a good idea. As soon as multiple threads are trying to use the connection at the same time, things are going to go wrong. The connections are already pooled for you (See here) so you creating and disposing IDbConnections is cheap and easy. ...


7

public interface IUnitOfWork : IDisposable { IRepository<T> Repository<T>() where T : class; void Commit(); void Rollback(); } The goal of a unit of work is to abstract the disposable stuff that's implementing it. By making your interface extend IDisposable, you've made a leaky abstraction - now anyone (including your mocking ...


7

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> {...


7

Did not use EF, stored procedures already available and VS2008 is limited to EF 3/4. A quick Google search for "SqlDatabase class" yields an outdated MSDN page about an obscure class in an obscure Microsoft.Practices.EnterpriseLibrary.Data.Sql namespace: Retired Content This content is outdated and is no longer being maintained. It is provided as a ...


6

To keep your repositories independent of the DBMS, you should inject the IDbConnection as parameter inside your constructor. IRepository<T> You should either rename the GetItem() to Get() or rename the other methods to XxItem() to keep the method names consistent. StateRepository I would rename the input parameter of the Create() method from ...


6

This code is terrible. Do not ever write this code again. This "pattern" is by far the most repeated failure of design I've seen. I'll also be the first admit in the past I used the EXACT same code when I was a junior developer. What is fundamentally wrong is abstract class Repository. This code is forcing inheritance for no reason. You are not going to ...


6

There is no general rule for using IEnumerable or not but there is one that says that you should use the most abstract representation of something because it gives you the most felxibility in what you can pass to such method because there would be fewer restrictions. This means if you are iterating a collection and you do this only once then IEnumerable<...


6

Errors An assumption: you never ever want to return a 500 HTTP status code (Internal server error). It may leak information you do not want to expose and it's not informative enough. Do not throw any exception: return the correct HTTP status code and eventually supplement it with the an appropriate error message. For example if SaveAll() failed because a ...


5

Maybe I'm just thinking of a different approach, but I normally would've reversed the dependency between UserEntity and UserRepository. I would expect the UserEntity to represent a singular record in the database and that I would use the UserRepository to retrieve one. In this case, I would inject the UserRepository into the controller and call ...


5

Testability Your repository implements an interface which will allow it to get stubbed out easily, so that's a very testable thing. However if you want to also (unit-test) your repositories themselves then you are stuck because you have a hardcoded dependency on DatabaseContext. You should move that up one layer by abstracting it out as well and providing ...


5

private static ICollection<State> PopulateStates() { return _db.Query<State>("SELECT * FROM States").ToList(); //Dapper ORM Code } Setting aside the issue TopinFrassi brought up of whether you should cache your entire table at all, each of your public methods are returning either a State object, a bool, or an IEnumberable<State> ...


5

#region #region CreateTenant Get rid of those. The region encompasses only a single method anyway, and your IDE should already allow you to collapse method scopes. Even if the region had more content, I'd recommend against it. See: Are #regions an antipattern or code smell? comments //open the connection this.connection.Open(); //start the ...


5

I wouldn't worry too much about the amount of things that aren't supported (yet/if ever) - it's impossible to cover everything in a scenario like this. One thing I would suggest is that you throw exceptions so the caller knows they're doing something unexpected: protected override Expression VisitMethodCall(MethodCallExpression m) { if (m.Method....


5

What we are looking at is a long tutorial on Inversion of Control and dependency injection. Basically at the most simpilest form you will be looking to abstract your data layer from you application layer and provide interfaces for your IoC container to deliver to the application. This can get quite confusing however all we have to do is remeber our ...


4

Is there a reason why others instantiate their transaction per unit of work? Yes, this is because more often than not, a unit of work is a single transaction, either it's committed, or it's not. Committing part of the work and not other parts, is not really a single unit. Additionally, Unit of Work is used frequently in web development where one unit = ...


4

Two things i notied: Wrap the transaction in a using-statement and also log the exception. // Performs the entire specified action within a single unit of work. private void WithinTransaction(Action action) { using(var transaction = session.BeginTransaction()){ try { action(); ...


4

Your code seems really good to me, I don't have anything else to say but that I'd remove the OnModelCreating override, as it has no purposes.


4

Unit of work There are some issues of mechanics here. Normally with a generic repository, you want to be able to extend it, so you'd write: public interface IOrderRepository : IGenericRepository<Order> { //Some Order-specific queries here } But this would mean updating your IUnitOfWork every time too. You're also having to add lots of annoying ...


4

Bad idea...? Generally, I belong to the camp thinking generic repositories are a bad idea: The abstraction will be leaky at best. If you really need a repository per type, why not let the client new it up directly? Except from basic CRUD, there is no generic functionality, so a generic interface will reduce your options. The abstractions doesn't seem ...


4

One major drawback... public void Insert(T entity) { context.Set<T>().Add(entity); context.SaveChanges(); } ...is that you've just destroyed the usefulness of a unit of work. DbContext wraps a database transaction, and what's great about transactions, is that they allow you to commit a "set" of changes all at once. But you're calling ...


4

First I'd like to start off by saying that if that code suits you, then it's not a bad idea to stick with it. It's fairly clean and pretty neat. I'm just going to point out a few things that may diverge from different types of implementation. Let's talk about the store and update methods. You are injecting a Reqeust class in it, which means your repository ...


4

Let's start with class coupling. This line this.connection = new DbConnectionHelper.DbConnectionHelper().GetDbConnection(); is a dependency of your class and should instead be managed and injected by the caller (preferably in the constructor) like so public class UserUnitOfWork { private readonly IDbConnection connection; public UserUnitOfWork(...


Only top voted, non community-wiki answers of a minimum length are eligible