2
\$\begingroup\$

Continuation of this question: https://codereview.stackexchange.com/questions/36561/unit-of-work-with-generic-repository-pattern-mvvm-vol-2

I have made some modifications:

  • Implement lazy-loading (hope so)
  • Add() and Update() returns T instead of void
  • Add IFeedItemRepository, FeedItemRepository, IBaseRepository<T>

IBaseRepositry

public interface IBaseRepository<T> where T : class
{
    int Count { get; }
    T Add(T entity);
    T GetById(int id);
    T Update(T entity);
    IEnumerable<T> GetAll();
    void Remove(T entity);
    void RemoveById(int id);
}

IBaseFeed

public interface IBaseFeed
{
    int Id { get; set; }
    string Title { get; set; }
    DateTime PubDate { get; set; }
    Uri Link { get; set; }
    string Misc { get; set; }
}

IFeedRepository

internal interface IFeedRepository<T> : IBaseRepository<T> where T : class, IBaseFeed
{
    int GetFeedIdByLink(string feedLink);
    int GetFeedIdByLink(Uri feedLink);
    T GetByLink(string feedLink);
    T GetByLink(Uri feedLink);
}

FeedRepository

internal class FeedRepository<T> : IFeedRepository<T>, IDisposable
    where T : class, IBaseFeed, new()
{
    private readonly SQLiteConnection _db;
    private Lazy<IList<T>> _feeds;

    private bool _isDisposed = false;

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

    public FeedRepository(SQLiteConnection db)
    {
        this._db = db;
        this._feeds = new Lazy<IList<T>>(() => _db.Table<T>().ToList());
    }

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

    public IEnumerable<T> GetAll()
    {
        return this._feeds.Value;
    }

    public T GetById(int id)
    {
        return this._feeds.Value.Where(feed => int.Equals(feed.Id, id)).Single();
    }

    public T GetByLink(string feedLink)
    {
        return this.GetByLink(new Uri(feedLink));
    }

    public T GetByLink(Uri feedLink)
    {
        return this._feeds.Value.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.Value.Where(feed => Uri.Equals(feed.Link, feedLink)).Select(feed => feed.Id).Single();
    }

    public void Remove(T feed)
    {
        this._db.Delete(feed);
    }

    public void RemoveById(int id)
    {
        this.Remove(this.GetById(id));
    }

    public T Update(T feed)
    {
        this._db.Update(feed);
        return feed;
    }

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

IFeedItemRepository

internal interface IFeedItemRepository : IFeedRepository<FeedItem>
{
    IEnumerable<FeedItem> GetFeedItemsByFeedDataId(int feedDataId);
}

FeedItemRepository

internal sealed class FeedItemRepository : FeedRepository<FeedItem>, IFeedItemRepository
{
    public FeedItemRepository(SQLiteConnection db)
        : base(db) { }

    public IEnumerable<FeedItem> GetFeedItemsByFeedDataId(int feedDataId)
    {
        return base.GetAll().Where(feed => int.Equals(feed.FeedDataId, feedDataId));
    }
}

IUnitOfWork

internal interface IUnitOfWork
{
    void Commit();
    void Rollback();
}

UnitOfWork

public class UnitOfWork : IUnitOfWork, IDisposable
{
    private readonly SQLiteConnection _db;

    private IFeedRepository<FeedData> _feedDataRepository;
    private IFeedItemRepository _feedItemRepository;

    private bool _isDisposed = false;

    public IFeedRepository<FeedData> FeedDataRepository
    {
        get
        {
            return _feedDataRepository;
        }
        set
        {
            _feedDataRepository = value;
        }
    }

    public IFeedItemRepository FeedItemRepository
    {
        get
        {
            return _feedItemRepository;
        }
        set
        {
            _feedItemRepository = value;
        }
    }

    public UnitOfWork(SQLiteConnection db)
    {
        this._db = db;
        this._feedDataRepository = new FeedRepository<FeedData>(this._db);
        this._feedItemRepository = new FeedItemRepository(this._db);

        this._db.BeginTransaction();
    }

    public void Commit()
    {
        this._db.Commit();
    }

    public void Rollback()
    {
        this._db.Rollback();
    }

    protected virtual void Dispose(bool isDisposing)
    {
        if (!this._isDisposed)
        {
            if (isDisposing)
            {
                if (this._feedDataRepository != null)
                {
                    ((FeedRepository<FeedData>)this._feedDataRepository).Dispose();
                }

                if (this._feedItemRepository != null)
                {
                    ((FeedItemRepository)this._feedItemRepository).Dispose();
                }

                if (this._db != null)
                {
                    this._db.Dispose();
                }
            }

            this._isDisposed = true;
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

Has the implementation of lazy-loading been done correctly?

What about this (UoW):

    private IFeedRepository<FeedData> _feedDataRepository;
    private IFeedItemRepository _feedItemRepository;

Is it correct?

Is the constructor in FeedItemRepository done properly?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Few tiny items:

  1. Can you use an IDbConnection interface (or worst case, DbConnection class) instead of SQLiteConnection for your constructor parameter/class member? This would allow for more generic operation, easier unit testing and easier swap-out of database providers in the future. This is in both the UnitOfWork class and the FeedRepository<T> class. Upon further reflection, the way that particular SQLite client library is used, this is unfeasible at this time. Still, I'd recommend a different SQLite client if at all possible which conforms to IDbConnection, etc.

  2. I would advise against Dispose()ing of the _db in the UnitOfWork class' Dispose() method since the class doesn't create it - the caller does. Let the caller Dispose() of it. Same goes for the FeedRepository<T> class.

  3. Do the FeedDataRepository and FeedItemRepository properties in the UnitOfWork class really need setters? It seems dangerous to create them in the constructor and then Dispose() them in the Dispose() method when someone could come along and change either to something else entirely. You could lose the original references altogether. I'd wager these should be invariant once constructed.

\$\endgroup\$
9
  • \$\begingroup\$ AFAIK there ISN't different SQLite client for WinRT, am I right? \$\endgroup\$
    – rechandler
    Jan 9, 2014 at 19:04
  • \$\begingroup\$ Got me on that front. No idea. \$\endgroup\$ Jan 9, 2014 at 19:17
  • 1
    \$\begingroup\$ This was a you-tell-me sort of thing. Do you need the setters? They seemed smelly to me considering the backing fields are created in the class. Are you assigning to them somewhere in client code? \$\endgroup\$ Jan 9, 2014 at 19:21
  • 1
    \$\begingroup\$ Same question applies - are you going to set via the property from within the class? Currently, it's not being done. Assignment is only to the backing member variable in the constructor. \$\endgroup\$ Jan 9, 2014 at 19:35
  • 1
    \$\begingroup\$ Then remove the setters and mark the fields as readonly. \$\endgroup\$ Jan 9, 2014 at 19:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.