RSS reader and model (logic) layer in practise

Question

I have written a logic part in my app. I'd like to learn best practices, so can you please check my class?

This class is a little too big in my opinion, but I have no idea what I should do now. I've pasted only the main class, but if you'd like to see anything related, please tell me.

This class is my logic in model layer. I think it is correct, am I right?

using RssReader.DataAccessLayer;
using RssReader.DataAccessLayer.Repository;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Threading;
using System.Threading.Tasks;
using Windows.Web;
using Windows.Web.Syndication;
namespace RssReader.Model.Logic
{
 public class FeedDataSource : IDisposable
 {
 private readonly SQLiteConnection _dbConnection;
 private readonly IUnitOfWork _unitOfWork;
 private readonly IRepository<FeedData> _feedDataRepository;
 private readonly IRepository<FeedItem> _feedItemRepository;
 private readonly RoamingSettings _roamingSettings;
 private readonly string _feedsLinkSettingNameInRoamingSettings;
 private bool _isDisposed;
 public FeedDataSource(string databaseName,
 string feedsLinkSettingNameInRoamingSettings)
 {
 this._dbConnection = InitializeDatabase(databaseName);
 this._unitOfWork = new UnitOfWork(this._dbConnection);
 this._feedDataRepository = this._unitOfWork.GetFeedRepository<FeedData>();
 this._feedItemRepository = this._unitOfWork.GetFeedRepository<FeedItem>();
 this._roamingSettings = new RoamingSettings();
 this._feedsLinkSettingNameInRoamingSettings = feedsLinkSettingNameInRoamingSettings;
 }
 public async Task<FeedData> AddFeedAsync(string feedLink)
 {
 return await this.AddFeedAsync(new Uri(feedLink));
 }
 public async Task<IEnumerable<FeedData>> GetFeedsAsync()
 {
 await this.CheckDatabaseIntegrity();
 IEnumerable<Uri> feedsLink = this.GetFeedsLinkFromRoamingSettings()
 .Select(link => new Uri(link));
 IEnumerable<string> eTags = (await Task.WhenAll
 (
 feedsLink.Select(link => GetETagAsync(link))
 )).Where(eTag => !string.Equals(eTag, string.Empty));
 IEnumerable<FeedData> refreshedFeeds = await Task.WhenAll
 (
 this._feedDataRepository
 .Find(feed => !eTags.Contains(feed.ETag))
 .Select(this.RefreshFeedAsync)
 );
 IEnumerable<FeedData> feedsData = refreshedFeeds.Union(this._feedDataRepository.GetAll());
 foreach (FeedData feedData in feedsData.Except(refreshedFeeds))
 {
 var feedItems = this._feedItemRepository
 .Find(feed => int.Equals(feed.FeedDataId, feedData.Id));
 foreach (FeedItem feedItem in feedItems)
 feedData.Items.Add(feedItem);
 }
 return feedsData;
 }
 public void DeleteFeed(string feedLink)
 {
 this.DeleteFeed(new Uri(feedLink));
 }
 public void SaveChanges()
 {
 this._unitOfWork.Commit();
 }
 public void RollbackChanges()
 {
 this._unitOfWork.Rollback();
 }
 public void Dispose()
 {
 this.Dispose(true);
 GC.SuppressFinalize(this);
 }
 private IEnumerable<string> GetFeedsLinkFromRoamingSettings()
 {
 object obj = this._roamingSettings
 .ReadValue(this._feedsLinkSettingNameInRoamingSettings);
 var collection = obj as IEnumerable<object> ?? Enumerable.Empty<object>();
 IEnumerable<string> feedsLinkFromRoamingSettings = Enumerable.Empty<string>();
 if (collection.Any())
 feedsLinkFromRoamingSettings = collection
 .Select(feedLink => feedLink.ToString()).Cast<string>();
 return feedsLinkFromRoamingSettings;
 }
 private async Task CheckDatabaseIntegrity()
 {
 IEnumerable<Uri> feedsLinkFromDatabase =
 this._feedDataRepository.GetAll().Select(feed => feed.Link);
 IEnumerable<Uri> feedsLinkFromRoamingSettings =
 this.GetFeedsLinkFromRoamingSettings().Select(link => new Uri(link));
 var matches = feedsLinkFromRoamingSettings.Except(feedsLinkFromDatabase);
 await Task.WhenAll
 (
 matches.Select(this.AddFeedAsync)
 );
 matches = feedsLinkFromDatabase.Except(feedsLinkFromRoamingSettings);
 foreach (Uri feedLink in matches)
 this.DeleteFeed(feedLink);
 }
 private async Task<FeedData> AddFeedAsync(Uri feedLink)
 {
 var matches = this._feedDataRepository.Find(feed => Uri.Equals(feed.Link, feedLink));
 if (matches.Any())
 {
 FeedData feedDataFromDatabase = matches.Single();
 feedDataFromDatabase.Error = FeedError.FeedAlreadyExist;
 return feedDataFromDatabase;
 }
 var cts = new CancellationTokenSource();
 Task<string> eTag = GetETagAsync(feedLink, cts);
 FeedData feedData = await RetrieveFeedAsync(feedLink);
 if (feedData == null)
 {
 cts.Cancel();
 return new FeedData { Error = FeedError.DownloadFeedError };
 }
 ICollection<string> feedsLinkFromRoamingSettings =
 this.GetFeedsLinkFromRoamingSettings().ToList();
 if (!feedsLinkFromRoamingSettings
 .Where(link => string.Equals(link, feedLink.ToString())).Any())
 {
 feedsLinkFromRoamingSettings.Add(feedLink.ToString());
 this._roamingSettings.AddOrUpdateValue
 (
 this._feedsLinkSettingNameInRoamingSettings,
 feedsLinkFromRoamingSettings.ToArray()
 );
 }
 feedData.ETag = await eTag;
 this._feedDataRepository.Add(feedData);
 foreach (FeedItem feedItem in feedData.Items)
 {
 feedItem.FeedDataId = feedData.Id;
 this._feedItemRepository.Add(feedItem);
 }
 return feedData;
 }
 private async Task<FeedData> RefreshFeedAsync(FeedData oldfeedData)
 {
 var cts = new CancellationTokenSource();
 Task<string> eTag = GetETagAsync(oldfeedData.Link, cts);
 FeedData feedData = await RetrieveFeedAsync(oldfeedData.Link);
 if (feedData == null)
 {
 cts.Cancel();
 oldfeedData.Error = FeedError.DownloadFeedError;
 return oldfeedData;
 }
 feedData.Id = oldfeedData.Id;
 feedData.Category = oldfeedData.Category;
 feedData.Order = oldfeedData.Order;
 feedData.ETag = await eTag;
 this._feedDataRepository.Update(feedData);
 var feedsToRemove = this._feedItemRepository
 .Find(feed => int.Equals(feed.FeedDataId, feedData.Id))
 .Except(feedData.Items, FeedComparer<FeedItem>.Instance)
 .ToList();
 foreach (FeedItem feedItem in feedsToRemove)
 this._feedItemRepository.Remove(feedItem);
 var feedsToAdd = feedData.Items.Except
 (
 this._feedItemRepository
 .Find(feed => int.Equals(feed.FeedDataId, feedData.Id)),
 FeedComparer<FeedItem>.Instance
 )
 .ToList();
 foreach (FeedItem feedItem in feedsToAdd)
 {
 feedItem.FeedDataId = feedData.Id;
 this._feedItemRepository.Add(feedItem);
 }
 return feedData;
 }
 private void DeleteFeed(Uri feedLink)
 {
 ICollection<string> feedsLinkFromRoamingSettings =
 this.GetFeedsLinkFromRoamingSettings().ToList();
 if (feedsLinkFromRoamingSettings
 .Where(link => string.Equals(link, feedLink.ToString())).Any())
 {
 feedsLinkFromRoamingSettings.Remove(feedLink.ToString());
 this._roamingSettings.AddOrUpdateValue
 (
 this._feedsLinkSettingNameInRoamingSettings,
 feedsLinkFromRoamingSettings.ToArray()
 );
 }
 int feedDataId = this._feedDataRepository
 .Find(feed => Uri.Equals(feed.Link, feedLink))
 .Select(feed => feed.Id).SingleOrDefault();
 this._feedDataRepository.RemoveById(feedDataId);
 IEnumerable<FeedItem> feedItems = this._feedItemRepository.GetAll()
 .Where(feed => int.Equals(feed.FeedDataId, feedDataId)).ToList();
 foreach (FeedItem feedItem in feedItems)
 this._feedItemRepository.Remove(feedItem);
 }
 private static SQLiteConnection InitializeDatabase(string databaseName)
 {
 string[] path ={ Windows.Storage.ApplicationData.Current.LocalFolder.Path,
 databaseName };
 var dbConnection =
 new SQLiteConnection(System.IO.Path.Combine(path), true);
 dbConnection.CreateTable<FeedData>(CreateFlags.ImplicitPK |
 CreateFlags.AutoIncPK);
 dbConnection.CreateTable<FeedItem>(CreateFlags.AllImplicit |
 CreateFlags.AutoIncPK);
 return dbConnection;
 }
 public static async Task<FeedData> RetrieveFeedAsync(Uri feedLink)
 {
 var feed = new SyndicationFeed();
 var client = new SyndicationClient();
 var feedData = new FeedData();
 client.SetRequestHeader("User-Agent", "Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.2; WOW64; Trident/6.0)");
 client.BypassCacheOnRetrieve = true;
 try
 {
 feed = await client.RetrieveFeedAsync(feedLink);
 }
 catch (Exception e)
 {
 SyndicationErrorStatus syndicationError = SyndicationError.GetStatus(e.HResult);
 if (syndicationError == SyndicationErrorStatus.Unknown)
 {
 WebErrorStatus webError = WebError.GetStatus(e.HResult);
 if (webError == WebErrorStatus.Unknown)
 throw;
 }
 return null;
 }
 if (feed.Title != null && feed.Title.Text != null)
 feedData.Title = feed.Title.Text;
 if (feed.Subtitle != null && feed.Subtitle.Text != null)
 feedData.Description = feed.Subtitle.Text;
 feedData.Link = feedLink;
 if (feed.Items != null && feed.Items.Any())
 {
 // Use the date of the latest post as the last updated date
 feedData.PubDate = feed.Items.First().PublishedDate.DateTime;
 foreach (SyndicationItem item in feed.Items)
 {
 FeedItem feedItem = new FeedItem();
 if (item.Title != null && item.Title.Text != null)
 feedItem.Title = item.Title.Text;
 if (item.PublishedDate != null)
 feedItem.PubDate = item.PublishedDate.DateTime;
 if (item.Authors != null && item.Authors.Any())
 feedItem.Author = item.Authors.First().Name;
 if (feed.SourceFormat == SyndicationFormat.Atom10)
 {
 if (item.Content != null && item.Content.Text != null)
 feedItem.Content = item.Content.Text;
 if (item.Id != string.Empty)
 feedItem.Link = new Uri(item.Id);
 }
 else if (feed.SourceFormat == SyndicationFormat.Rss20)
 {
 if (item.Summary != null && item.Summary.Text != null)
 feedItem.Content = item.Summary.Text;
 if (item.Links != null && item.Links.Any())
 feedItem.Link = item.Links.First().Uri;
 }
 feedData.Items.Add(feedItem);
 }
 }
 return feedData;
 }
 private static async Task<string> GetETagAsync(Uri feedLink,
 CancellationTokenSource cts = null)
 {
 const int millisecondsTimeout = 2500;
 if (cts == null)
 cts = new CancellationTokenSource();
 WebRequest webRequest = WebRequest.Create(feedLink);
 webRequest.Method = "HEAD";
 webRequest.Headers["UserAgent"] = "Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.2; WOW64; Trident/6.0)";
 Task<WebResponse> webResponse = WebRequestExtensions.GetResponseAsync(webRequest, cts.Token);
 try
 {
 if (await Task.WhenAny(webResponse,
 Task.Delay(millisecondsTimeout)) == webResponse)
 {
 using (var result = await webResponse)
 {
 return result.Headers["ETag"] ?? string.Empty;
 }
 }
 else
 {
 cts.Cancel();
 return string.Empty;
 }
 }
 catch (WebException)
 {
 return string.Empty;
 }
 }
 protected virtual void Dispose(bool isDisposing)
 {
 try
 {
 if (!this._isDisposed)
 {
 if (isDisposing)
 {
 if (this._dbConnection != null)
 this._dbConnection.Dispose();
 }
 }
 }
 finally
 {
 this._isDisposed = true;
 }
 }
 }
 public static class WebRequestExtensions
 {
 public static async Task<WebResponse> GetResponseAsync(this WebRequest request,
 CancellationToken ct)
 {
 using (ct.Register(() => request.Abort(), useSynchronizationContext: false))
 {
 try
 {
 var response = await request.GetResponseAsync();
 ct.ThrowIfCancellationRequested();
 return (WebResponse)response;
 }
 catch (WebException e)
 {
 if (e.Status == WebExceptionStatus.RequestCanceled)
 ct.ThrowIfCancellationRequested();
 if (ct.IsCancellationRequested)
 throw new TaskCanceledException(e.Message, e);
 throw;
 }
 }
 }
 }
}

Answer 1

Your constructor has mixed concerns:

public FeedDataSource(string databaseName,
 string feedsLinkSettingNameInRoamingSettings)
{
 this._dbConnection = InitializeDatabase(databaseName);
 this._unitOfWork = new UnitOfWork(this._dbConnection);
 this._feedDataRepository = this._unitOfWork.GetFeedRepository<FeedData>();
 this._feedItemRepository = this._unitOfWork.GetFeedRepository<FeedItem>();
 this._roamingSettings = new RoamingSettings();
 this._feedsLinkSettingNameInRoamingSettings = feedsLinkSettingNameInRoamingSettings;
}

These lines increase the coupling between FeedDataSource and other classes:

 this._unitOfWork = new UnitOfWork(this._dbConnection);
 this._roamingSettings = new RoamingSettings();

If you have an abstraction for UnitOfWork and RoamingSettings, you should inject them as dependencies of your class:

public FeedDataSource(IUnitOfWork unitOfWork,
 IRoamingSettings roamingSettings,
 string feedsLinkSettingNameInRoamingSettings)
{
 _unitOfWork = unitOfWork;
 _roamingSettings = roamingSettings;
 //...
}

The this qualifier you're using, is redundant. If private fields have an underscore prefix, it's specifically to avoid having to use this to qualify a member. I would drop it, the code would be easier on the eyes.

I like that your private fields are readonly.

---

This method is calling a private overload:

public async Task<FeedData> AddFeedAsync(string feedLink)
{
 return await this.AddFeedAsync(new Uri(feedLink));
}

I wouldn't bother with the overload if it's not public - in other words, if the only way your client code can/should AddFeedAsync is by providing a string, then I'd merge the two methods into a single public member.

However I like that you have an overload that takes an Uri parameter, so I'd make that overload public, and move the method closer (just below) the other AddFeedAsync overload. Right now you have them completely separated, normally I'd expect a method and its overloads to be regrouped, to show up close to each other in the code file.

Same goes for DeleteFeed.

---

There's something about RetrieveFeedAsync that smells:

foreach (SyndicationItem item in feed.Items)
{
 FeedItem feedItem = new FeedItem();
 if (item.Title != null && item.Title.Text != null)
 feedItem.Title = item.Title.Text;
 if (item.PublishedDate != null)
 feedItem.PubDate = item.PublishedDate.DateTime;
 if (item.Authors != null && item.Authors.Any())
 feedItem.Author = item.Authors.First().Name;
 if (feed.SourceFormat == SyndicationFormat.Atom10)
 {
 if (item.Content != null && item.Content.Text != null)
 feedItem.Content = item.Content.Text;
 if (item.Id != string.Empty)
 feedItem.Link = new Uri(item.Id);
 }
 else if (feed.SourceFormat == SyndicationFormat.Rss20)
 {
 if (item.Summary != null && item.Summary.Text != null)
 feedItem.Content = item.Summary.Text;
 if (item.Links != null && item.Links.Any())
 feedItem.Link = item.Links.First().Uri;
 }
 feedData.Items.Add(feedItem);
}

You should always enclose a scope with {} braces, even if it's just for a single instruction. Also I would extract the if...else part into its own method, to reduce nesting; I think you could have a SetFeedItemLinkAndContent(SyndicationItem item, FeedItem feedItem, SyndicationFormat format) method, which would make the loop look like this:

foreach (SyndicationItem item in feed.Items)
{
 FeedItem feedItem = new FeedItem();
 if (item.Title != null && !string.IsNullOrEmpty(item.Title.Text))
 {
 feedItem.Title = item.Title.Text;
 }
 if (item.PublishedDate != null)
 {
 feedItem.PubDate = item.PublishedDate.DateTime;
 }
 if (item.Authors != null && item.Authors.Any())
 {
 feedItem.Author = item.Authors.First().Name;
 }
 SetFeedItemLinkAndContent(item, feedItem, feed.SourceFormat);
 feedData.Items.Add(feedItem);
}

And the SetFeedItemLinkAndContent method could look like this:

private void SetFeedItemLinkAndContent(SyndicationItem item, 
 FeedItem feedItem, 
 SyndicationFormat format)
{
 string content = string.Empty;
 Uri link = string.Empty;
 switch (feed.SourceFormat)
 {
 case SyndicationFormat.Atom10:
 if (item.Content != null && !string.IsNullOrEmpty(item.Content.Text))
 {
 content = item.Content.Text;
 }
 if (!string.IsNullOrEmpty(item.Id))
 {
 link = new Uri(item.Id);
 }
 break;
 case SyndicationFormat.Rss20:
 if (item.Summary != null && !string.IsNullOrEmpty(item.Summary.Text))
 {
 content = item.Summary.Text;
 }
 if (item.Links != null && item.Links.Any())
 {
 link = item.Links.First().Uri;
 }
 break;
 case default:
 throw new NotSupportedException("Unhandled SyndicationFormat value.");
 }
 feedItem.Content = content;
 feedItem.Link = link;
}

You may want to consider extending the SyndicationItem type with a ToFeedItem extension method to host this logic instead, which would further simplify this code:

foreach (SyndicationItem item in feed.Items)
{
 var feedItem = item.ToFeedItem(feed.SourceFormat);
 feedData.Items.Add(feedItem);
}

---

Condition evaluations in C# are smarter than that. This:

if (!this._isDisposed)
{
 if (isDisposing)
 {
 if (this._dbConnection != null)
 this._dbConnection.Dispose();
 }
}

Is exactly the same as this:

if (!_isDisposed && isDisposing && _dbConnection != null)
{
 _dbConnection.Dispose();
}

isDisposing will not be evaluated if !_isDisposed evaluates to false, and _dbConnection != null will not be evaluated if any of the previous conditions already failed: don't nest 3 successive conditions 3 levels deep - make one condition with all 3 Boolean expressions instead.

---

Your class indeed has too many responsibilities. Normally a type that consumes a UnitOfWork does not care about the database stuff - the very idea of a unit of work / repository is to abstract the database connection away! By moving the instantiation of the unit of work outside the constructor like I recommended above (way above), you also remove InitializeDatabase from your class' concerns, and ask the code that instantiates a FeedDataSource to also instantiate a UnitOfWork - and since that code will be owning the database connection, you can probably also get rid of a lot of the Dispose logic (i.e. move it elsewhere).

• rechandler: Thanks a lot, I was helpful ;) Btw should I keep extensions class in other file, or can I have it in the same file as main (FeedDataSource) class?