Is this Repository Design Pattern Valid and efficient?

I am using Dapper ORM in my Data access Layer, but I think analysis of the code below doesn't depend upon any specific ORM. What should be the single common place throughout the entire Data Access Layer to initialise the Connection String?

Base Abstract Class

abstract class Repository
{
protected static IDbConnection _db = new SqlConnection(ConfigurationManager.ConnectionStrings["GuideDB"].ConnectionString);

//should I use Constructor here
}


A Generic Repository Interface

public interface IRepository<T>
{
IEnumerable<T> GetAll();
T GetItem(int id);
T Create(T item);
bool Update(T item);
bool Delete(int id);
void Reset();
}


Concrete class

public class StateRepository :Repository, IRepository<State>
{

private static ICollection<State> _states = PopulateStates();// doing this valid ??
private static object _lock = new object();

#region Public Methods

public IEnumerable<State> GetAll()
{
lock (_lock)
{
return _states.ToArray();
}
}

public IEnumerable<State> GetAllStatesByCountryId(short id)
{
lock (_lock)
{
return _states.Where(s => s.CountryId == id).ToArray();
}
}

public State GetItem(int id)
{
lock (_lock)
{
return _states.FirstOrDefault(s => s.Id == id);
}
}

public State Create(State state)
{
lock (_lock)
{
if (state == null)
{
throw new ArgumentNullException("state");
}

state.Id = (short)(_states.Any() ? _states.Max(s => s.Id) + 1 : 1);
_states.Add(state);
return state;
}
}

public bool Update(State item)
{
lock (_lock)
{
var state = _states.FirstOrDefault(s => s.Id == item.Id);

if (state != null)
{
state.Id = item.Id;
state.Name = item.Name;
state.ShortInfo = item.ShortInfo;
state.CountryId = item.CountryId;

return true;
}

return false;
}
}

public bool Delete(int id)
{
lock (_lock)
{
var state = _states.FirstOrDefault(s => s.Id == id);
return _states.Remove(state);
}
}

public void Reset()
{
lock (_lock)
{
_states = PopulateStates(); // doing this valid ??
}
}
#endregion

private static ICollection<State> PopulateStates()
{
return  _db.Query<State>("SELECT * FROM States").ToList(); //Dapper ORM Code
}

//How to Add a efficient Dispose method to this repository , is it really needed
}

• If you want a truly static cache use private static readonly if you want a cache that you can reset like you have with Reset() use HttpRuntime.Cache. In general if you need to write a lock, you're doing something wrong. Locking and threading is one of the most difficult concepts in all of programming, do everything possible to avoid locking before you decide you must use locking. – Chris Marisic Sep 26 '14 at 19:30

5 Answers

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 connection, because keeping the static connection might cause problems. What would happen if one of my Repository implementation would dispose the connection? All your repositories would be down.

abstract class SqlRepository<T> : IRepository<T>
{
protected IDbConnection Connection { get; set; }

protected SqlRepository<T>(string connectionString)
{
Connection = new SqlConnection(connectionString);
}

abstract IEnumerable<T> GetAll();
abstract T GetItem(int id);
abstract T Create(T item);
abstract bool Update(T item);
abstract bool Delete(int id);
abstract void Reset();
}


If your application runs on one instance, your repository would be okay. But since it puts all database entities in cache (your collection) and never returns to the database unless you specify it, you have a problem. Example : Client A (On an X machine) loads the database in cache, Client B (on a Y machine) does the same then adds a State to the repository. Client A calls GetAll, what should be the expected output? Client A wouldn't have the new State. And if Client B calls Reset the new State was never persisted, so it is lost.

I don't think you should use the ICollection<State>, each of your call should go to the database, so you are sure you have the latest version of your data. If you remove it, I don't think you need to handle thread safety anymore.

• Having the Connection setter private may convey intent a little bit better. – Matthew Sep 26 '14 at 18:04
• Yeah you're not wrong, it's just that since I don't know what the OP does in his derived classes, I decided not to include it in the review.. – IEatBagels Sep 26 '14 at 18:37
• -1 this code should not exist at all. It purely duplicates Dapper's inherently being the repository pattern. – Chris Marisic Sep 26 '14 at 19:18
• It acts as a proxy for the IRepository<T> interface, doesn't it? I don't know Dapper that much but if there's no efficient way to mock itself, then covering it with an interface will provide a good solution for unit testing and any kind of dependency injection – IEatBagels Sep 26 '14 at 19:24
• Plus, I'd say this code has nothing to do with Dapper. The implementation could be pure Sql.. – IEatBagels Sep 26 '14 at 19:25

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 state to (like in the other method) item. In this way it is more obvious that it is a method of the interface.

Otherwise your code looks fine for me.

• You're correct about injecting the connection string so it can be easily varied (such as in a integration test) but the rest of the code is worthless code that needs deleted. – Chris Marisic Sep 26 '14 at 19:20

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 act polymorphically across repositories such as

List<Repository> Repositories = new List<Repository> { ...... };
Repositories.ForEach(x=> x.Create(foo));


If you really have a need to operate across a list of Repositories polymorphically this is a great solution. I know you don't have that case though.

Why is abstract class Repository SO BAD?

Take your example StateRepository. States are very closely intertwined with Cities, Zipcodes, and Counties. Suppose you want to have a GeoRepository that provides you with a Zipcode's City, County and State? How are you going to achieve that with this code? C# does not support multiple inheritance so you can't have GeoRepository : ZipCodeRepository, StateRepository, CityRepository

So the next attempt would be something like:

GeoRepository : ZipCodeRepository : StateRepository : CityRepository


Even mentioning ZipCodeRepository : StateRepository : CityRepository shows the absurdity of this "pattern". ZipCodes don't inherit from State or City, but this is the type of relationship that will be forced abstract class Repository. Also it just won't work anyway because you'll keep overwriting the same methods. Suppose the code used Repository<T> then atleast the compiler will allow this whacky inheritance chain. It'd still be just as wrong though.

You mention Dapper.

Dapper is itself a repository. There is absolutely no reason to create abstraction to Dapper. The entire purpose of Dapper is minimize abstraction! Dapper was created as a solution to ORMs. ORMs are ultra-abstraction they require you to understand both the database fully and the ORM. Dapper is meant to provide the closest to metal experience as possible while avoiding the brittle mapping work that raw ADO.NET takes. It also allows access to run real sql, something most ORMs cannot do at all or do poorly.

To abstract away Dapper you're providing yourself the lowest common denominator instead of full the functionality of Dapper.

• meta.codereview.stackexchange.com/questions/2499/… – IEatBagels Sep 26 '14 at 20:09
• @TopinFrassi This code is terrible. This code was not dreamed up by Abhishek. It was not dreamed up by me. It has been promoted as "a good" by many many people over many years. I even blogged about this same code years ago because I was just as equally swindled. I wish someone years ago told me what I have the opportunity to tell Abhishek today. Also it seems like many others here need to see my statements given the all upvotes given to answers promoting this huge waste of effort and bad abstraction. – Chris Marisic Sep 26 '14 at 20:45
• Let's say I thought you were right, I still think your review is pretty harsh, this is all I meant when I put this comment in your answer – IEatBagels Sep 26 '14 at 20:49
• @TopinFrassi I slightly reworded my post to emphasize I am speaking down on this "pattern" that circulates the internet and not the intentions of the OP. My words are meant to be harsh, harsh to all the people that have mislead Abhishek, myself, and most (all?) people in this thread. – Chris Marisic Sep 26 '14 at 21:02
• For the record, it wasn't @TopinFrassi's downvote. It was mine. And it's no been removed since you directed your frustration at the code instead of OP. – RubberDuck Sep 26 '14 at 22:29
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> object. There is no reason to use ToList here.

Simply store your cache as an IEnumerable<State>, and stop calling ToArray in your GetAll* methods.

state.Id = item.Id;
state.Name = item.Name;
state.ShortInfo = item.ShortInfo;
state.CountryId = item.CountryId;


Assuming CountryId is a foreign key to something like a Countries table, I would recommend including public Country Country { get; set; } in your State class. Then, use Dapper's multi-map query to get both at the same time:

string sql = @"SELECT s.Id, s.Name, s.ShortInfo, s.CountryId,
c.Id, c.Name -- etc. for Country table
FROM States s
JOIN Countries c ON s.CountryId = c.Id";
return _db.Query<State, Country, State>(sql, (state, country) =>
{
state.Country = country;
return state;
});


Note that if the first column you're selecting for each object after the first isn't named "Id", you need to supply the splitOn parameter to the Query method. splitOn is a comma-delimited list of column names. If the first column for each object after the first (the first object doesn't matter, since it's obvious where it begins!) is named "Id", splitOn is not needed.

Including a Country property in your State object will make it easier to reference whatever country each state is in. Rather than hitting the database again, you simply get the country at the same time. Even if you choose to stop caching your states, you can use this strategy for your GetItem method.

string sql = @"SELECT s.Id, s.Name, s.ShortInfo, s.CountryId,
c.Id, c.Name -- etc. for Country table
FROM States s
JOIN Countries c ON s.CountryId = c.Id
WHERE s.Id = @Id";
return _db.Query<State, Country, State>(sql, (state, country) =>
{
state.Country = country;
return state;
}, new { Id = id }).SingleOrDefault();


You may also notice in the above that I'm specifying each column to select individually, rather than using *. There are arguments both ways, especially for an ORM (or pseudo-ORM like Dapper) trying to pick up the entire object, but I much prefer laying everything out. This helps make it clear within the repository code what columns are available to me, permits me to easily omit columns from the model object, and can make it easier to map the same table to potentially different models if the project needs that for some reason. Also, if the database schema changes, Dapper can throw an exception, which will draw my attention to the fact that the model needs changing.

You'll also see in my second example that I'm using SingleOrDefault instead of FirstOrDefault. I am making the assumption here that Id is a primary key (or potentially a unique index), and so the SQL result set should never produce multiple records. (If that assumption is wrong, of course, you should stick with FirstOrDefault.)

If I'm calling Update on a State that doesn't exist, I would expect an exception to be thrown, not merely have false returned. This is how I'd write it:

    public void Update(State item)
{
lock (_lock)
{
var state = _states.Single(s => s.Id == item.Id);

state.Id = item.Id;
state.Name = item.Name;
state.ShortInfo = item.ShortInfo;
state.CountryId = item.CountryId;
}
}

• I'm not sure wether InvalidOperationException is clear enough, maybe should it throw a custom exception? – IEatBagels Sep 26 '14 at 18:39
• There's no need to create any type of custom exception for this. The only reason to create a custom exception is for example take UpdatedItemAlreadyDeletedException. The only reason to ever create UpdatedItemAlreadyDeletedException is that your system is able to able to compensate and recover from this exception. Perhaps by going to history tables, undeleting the item, and retrying to the update. If you have zero compensation logic involved, just use whatever built in type is good enough InvalidOperationException, ApplicationException, etc. – Chris Marisic Sep 26 '14 at 19:38
• Reading this closer, the real issue is using Single() the exception it provides is absolutely worthless. In the past I've created an extension method to "overload" Single that would allow me to supply the exception message. It would make no sense if you call Update and get an exception "list contained no items" – Chris Marisic Sep 26 '14 at 19:50