Unit of Work with Generic Repository Pattern MVVM

Question

I have been writing the application. I meant to implement a repository pattern with Unit of Work. Is it correctly done? Can you make a code review? I use SQLite wrapper sqlite-net.

IFeedRepository:

internal interface IFeedRepository<T> : IDisposable where T : IBaseFeed
{
 int Count { get; }
 void Add(T feed);
 IEnumerable<T> GetAll();
 T GetFeedById(int id);
 T GetFeedByLink(string feedLink);
 T GetFeedByLink(Uri feedLink);
 int GetFeedIdByLink(string feedLink);
 int GetFeedIdByLink(Uri feedLink);
 void Remove(T feed);
 void RemoveById(int id);
 void Update(T feed);
 void Save();
}

FeedRepository:

internal class FeedRepository<T> : IFeedRepository<T> where T: IBaseFeed, new()
{
 private readonly SQLiteConnection _db;
 private IList<T> _feeds;
 private bool _isDisposed = false;
 public int Count
 {
 get
 {
 return _feeds.Count;
 }
 }
 public FeedRepository(SQLiteConnection db)
 {
 this._db = db;
 this._feeds = _db.Table<T>().ToList();
 this._db.BeginTransaction();
 }
 public void Add(T feed)
 {
 this._feeds.Add(feed);
 this._db.Insert(feed);
 }
 public IEnumerable<T> GetAll()
 {
 return this._feeds;
 }
 public T GetFeedById(int id)
 {
 return this._feeds.Where(feed => int.Equals(feed.Id, id)).Single();
 }
 public T GetFeedByLink(string feedLink)
 {
 return this.GetFeedByLink(new Uri(feedLink));
 }
 public T GetFeedByLink(Uri feedLink)
 {
 return this._feeds.Where(feed => Uri.Equals(feed.Link, feedLink)).Single();
 }
 public int GetFeedIdByLink(string feedLink)
 {
 return this.GetFeedIdByLink(new Uri(feedLink));
 }
 public int GetFeedIdByLink(Uri feedLink)
 {
 return this._feeds.Where(feed => Uri.Equals(feed.Link, feedLink)).Select(feed => feed.Id).Single();
 }
 public void Remove(T feed)
 {
 this._feeds.Remove(feed);
 this._db.Delete(feed);
 }
 public void RemoveById(int id)
 {
 this.Remove(this.GetFeedById(id));
 }
 public void Update(T feed)
 {
 int indexOfFeed = this._feeds.IndexOf(this.GetFeedById(feed.Id));
 this._feeds[indexOfFeed] = feed;
 this._db.Update(feed);
 }
 public void Save()
 {
 this._db.Commit();
 }
 public void Dispose()
 {
 Dispose(true);
 GC.SuppressFinalize(this);
 }
 protected virtual void Dispose(bool isDisposing)
 {
 if (!this._isDisposed)
 {
 if (isDisposing)
 {
 if (this._db != null)
 {
 this._db.Dispose();
 }
 }
 this._isDisposed = true;
 }
 }
 ~FeedRepository()
 {
 this.Dispose(false);
 }
}

And Unit of Work, that is probably wrong :/

IUnitOfWork:

public interface IUnitOfWork : IDisposable
{
 int Count { get; }
 void Add(FeedData feedData);
 IEnumerable<FeedData> GetAll();
 FeedData GetFeedDataById(int feedDataId);
 FeedData GetFeedDataByLink(string feedDataLink);
 FeedData GetFeedDataByLink(Uri feedDataLink);
 void Remove(FeedData feedData);
 void RemoveByFeedDataId(int feedDataId);
 void Update(FeedData feedData);
 void Save();
}

UnitOfWork:

public class UnitOfWork : IUnitOfWork
{
 private readonly SQLiteConnection _db;
 private IList<FeedData> _feedsData;
 private FeedRepository<FeedData> _feedDataRepository;
 private FeedRepository<FeedItem> _feedItemRepository;
 private bool _isDisposed = false;
 public int Count
 {
 get
 {
 return _feedsData.Count;
 }
 }
 public UnitOfWork(SQLiteConnection db)
 {
 this._db = db;
 this._feedDataRepository = new FeedRepository<FeedData>(this._db);
 this._feedItemRepository = new FeedRepository<FeedItem>(this._db);
 foreach (FeedData feedData in this._feedDataRepository.GetAll())
 {
 feedData.Items = this._feedItemRepository.GetAll().Where(item => int.Equals(item.FeedDataId, feedData.Id)).ToList();
 this._feedsData.Add(feedData);
 }
 }
 public void Add(FeedData feedData)
 {
 this._feedsData.Add(feedData);
 this._db.BeginTransaction();
 this._feedDataRepository.Add(feedData);
 foreach (FeedItem item in feedData.Items)
 {
 item.FeedDataId = feedData.Id;
 this._feedItemRepository.Add(item);
 }
 this._db.Commit();
 }
 public IEnumerable<FeedData> GetAll()
 {
 return this._feedsData;
 }
 public FeedData GetFeedDataById(int feedDataId)
 {
 return this._feedsData.Where(feed => int.Equals(feed.Id, feedDataId)).Single();
 }
 public FeedData GetFeedDataByLink(string feedDataLink)
 {
 return this.GetFeedDataByLink(new Uri(feedDataLink));
 }
 public FeedData GetFeedDataByLink(Uri feedDataLink)
 {
 return this._feedsData.Where(feed => Uri.Equals(feed.Link, feedDataLink)).Single();
 }
 public void Remove(FeedData feedData)
 {
 this._feedsData.Remove(feedData);
 this._db.BeginTransaction();
 this._feedDataRepository.Remove(feedData);
 foreach (FeedItem feedItem in feedData.Items)
 {
 this._feedItemRepository.Remove(feedItem);
 }
 this._db.Commit();
 }
 public void RemoveByFeedDataId(int feedDataId)
 {
 FeedData feedData = this._feedDataRepository.GetFeedById(feedDataId);
 feedData.Items = this._feedItemRepository.GetAll().Where(item => int.Equals(item.FeedDataId, feedData.Id)).ToList();
 this.Remove(feedData);
 }
 public void Update(FeedData feedData)
 {
 var matches = this._feedsData.Where(feed => int.Equals(feed.Id, feedData.Id));
 int indexOfFeed = this._feedsData.IndexOf(matches.Single());
 this._feedsData[indexOfFeed] = feedData;
 this._db.BeginTransaction();
 this._feedDataRepository.Update(feedData);
 foreach (FeedItem feedItem in feedData.Items)
 {
 this._feedItemRepository.Update(feedItem);
 }
 this._db.Commit();
 }
 protected virtual void Dispose(bool isDisposing)
 {
 if (!this._isDisposed)
 {
 if (isDisposing)
 {
 if (this._feedDataRepository != null)
 {
 this._feedDataRepository.Dispose();
 }
 if (this._feedItemRepository != null)
 {
 this._feedItemRepository.Dispose();
 }
 }
 this._isDisposed = true;
 }
 }
 public void Save()
 {
 this._feedDataRepository.Save();
 this._feedItemRepository.Save();
 }
 public void Dispose()
 {
 Dispose(true);
 GC.SuppressFinalize(this);
 }
 ~UnitOfWork()
 {
 this.Dispose(false);
 }
}

Is it clear? Or Shall I specify what I want to achieve?

• ChrisWue: From msdn.microsoft.com/en-us/library/…: AVOID making types finalizable. Carefully consider any case in which you think a finalizer is needed. There is a real cost associated with instances with finalizers, from both a performance and code complexity standpoint.

Answer 1

Your IUnitOfWork interface has mixed concerns with IFeedRepository; I don't think the GetFeedData methods belong in there. The way I understand UoW, it should be looking something like this:

public interface IUnitOfWork
{
 void Commit(); // save changes
 void Rollback(); // discard changes
}

..and vice-versa; if IFeedRepository.Save() has the same meaning as IUnitOfWork.Save(), then I don't think IFeedRepository interface should feature it.

An interface is an abstraction. An interface that forces its implementation to implement IDisposable is a leaky abstraction, because you're assuming all implementations of your repository interface will be disposable, which is an assumption waiting to be proven wrong: the implementation is leaking into the abstraction. What if you wanted to implement that repository with an IList<T> instead of a Sqlite backend?

Also I don't think the finalizers are needed; just call the Dispose() method, or wrap your instance in a using block instead.

---

I don't like that your UnitOfWork class constructor has the side-effect of hitting the database, but I do like the connection being owned by the caller and being constructor-injected into the UoW, and as a Unit of Work is actually supposed to encapsulate a transaction, I suggest you move the transaction stuff in there.

I think IFeedRepository<T> can be dropped altogether, since the UoW isn't using it - it's directly instanciating the concrete implementations and encapsulates them as concrete implementations, which leaves your interface unused.

Alternatively, you could have some IRepositoryFactory<T> with some IFeedRepository<T> Create(SQLiteConnection) method, constructor-injected into your UoW - this would decouple your UoW from the repository implementations, and make the interface serve a purpose. I prefer this alternative, because it's coding against an interface, not an implementation.

Now I just noticed this line in your repo constructor:

this._feeds = _db.Table<T>().ToList();

If you have 1,000,000 rows in that table, you have a very expensive constructor here. Again, it's a constructor with side-effects; I would only assign the connection reference in there.

In the end, I would try to keep it as simple as possible:

• UnitOfWork:
  • Owns (and exposes) repositories (owns, meaning it's its job to also dispose them)
  • Encapsulates transaction (commit/rollback)
• Repositories:
  • Provide an abstraction layer over the SQLite plumbing
  • Expose ways to perform CRUD operations

_{Note: this may be all wrong: I use entity-framework and don't bother with UoW/Repository patterns; my view of a UoW is probably biased with DbContext, which I've read some people consider a bad UoW implementation - yet nobody could clearly explain why.}

• ChrisWue: Calling GC.SuppressFinalize(yhis) is part of the basic pattern for using IDisposable (msdn.microsoft.com/en-us/library/…)
• Mathieu Guindon: @ChrisWue I thought all you had to do was implementing IDisposable.Dispose() with a method that calls Dispose() on all IDisposable resources you're holding.. wouldn't that be enough?
• ChrisWue: The idea is that if Dispose was called the resources have been cleaned up and there is not need to run the finalizer which reduces the amount of work the CLR has to do. If you have an unsealed class then someone can derive from it and possibly have a subclass implementing a finalizer. The default pattern will take care of it. In the end it's always good to use a pattern consistently.
• Mathieu Guindon: @Malachi using (var foo = new bar()) will not compile if bar isn't IDisposable.
• Mathieu Guindon: @J.Marciniak edited with more details :)