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?

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