Unit of Work with Generic Repository Pattern MVVM

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 : IDisposable where T : IBaseFeed
{
    int Count { get; }

    void Add(T feed);
    IEnumerable 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 : IFeedRepository where T: IBaseFeed, new()
{
private readonly SQLiteConnection _db;
private IList _feeds;
private bool _isDisposed = false;

public int Count
{
get
{
return _feeds.Count;
}
}

public FeedRepository(SQLiteConnection db)
{
this._db = db;
this._feeds = _db.Table().ToList();
this._db.BeginTransaction();
}

public void Add(T feed)
{
this._feeds.Add(feed);
this._db.Insert(feed);
}

public IEnumerable 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(fee

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 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 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 with some IFeedRepository 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().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.

StackExchange Code Review Q#36523, answer score: 6