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 ...
public interface IUnitOfWork : IDisposable
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, ...
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 ...
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 ...
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'...
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 ...
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:
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 ...
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.
public interface IUnitOfWork : IDisposable
IRepository<T> Repository<T>() where T : class;
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 ...
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>
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:
This content is outdated and is no longer being maintained. It is provided as a ...
To keep your repositories independent of the DBMS, you should inject the IDbConnection as parameter inside your constructor.
You should either rename the GetItem() to Get() or rename the other methods to XxItem() to keep the method names consistent.
I would rename the input parameter of the Create() method from ...
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 ...
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<...
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 ...
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 ...
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 ...
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> ...
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?
//open the connection
//start the transaction
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)
Initial and General Thoughts
Tests all passed
I like the project structure and naming
Source control - I find it easier to handle if packages not there, especially on a slow connection
Use nuget package restore
I generally use this .gitignore file for this type of project https://github.com/github/gitignore/blob/master/VisualStudio....
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 ...
You have a real interface pollution there. They do not provide any useful abstraction. Keep only one of them that you make generic. That's all you need.
public interface IRepository<T>
T GetById(int id);
void Add(T entity);
void Update(T entity);
void Delete(T entity);
I think you are in the right direction but let me suggest some things.
Remove the try catch and let the repository throw the exception. Either that or catch and throw your own exception.
Inject IDbConnection in the constructor of the repository and remove the using on the repository methods
private IDbConnection _connection;
I think you can DRY this up pretty well by refactoring.
This feels like a “ifs are evil” code smell to me. There are three “if” statements within that whereHas. They are packed in fairly tightly, and it makes that part of the code more difficult to read. Trying to refactor that as much as possible can resolve the duplication along the way, and ultimately ...
On its own, this:
private MySqlConnection _myDBString;
public IDbConnection GetMyDBConnection
_myDBString = new MySqlConnection(epaperconnectionString);
is a deeply bad idea. Getters should usually be trusted to have no side-effects that mutate the class.
The only exception to this I ...
the idea that an exception should be thrown in exceptional cases is very vague, for example, if I can't find a user in my data source (repository) I can't continue the flow of my code, can this be considered exceptional?
My personal approach is to raise an exception if I encounter a scenario where I make a truthy assumption during code execution, but that ...
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 = ...
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 ...