I was recently taking a look at the Kazi Manzur Kigg MVC implementation (Kazi rocks) and noticed some code that seemed to defeat the DRY/SOC principle. I'd love to have everyone's thoughts on a possible refactor to separate concerns.
Kigg implements both an Add and Remove method on each repository class ( Note: BaseRepository has virtual methods than can be overloaded by each concrete implementation.)
The implementations for the Kigg.Repository.LinqToSql.CategoryRepository and the Kigg.Repository.LinqToSql.StoryRepository both cascade deletes via their Remove methods in order to delete child entities. ( Note: Category has a parent relationship (one-to-many) to Story, so they share the same child relationships from Story down through the object graph) see diagram . The offending code is the way both repositories delete each others child entities:
CategoryRepository
namespace Kigg.Repository.LinqToSql.CategoryRepository{
//using statements omitted
public class CategoryRepository : BaseRepository<ICategory, Category>, ICategoryRepository
{
//code omitted
public override void Remove(ICategory entity)
{
Check.Argument.IsNotNull(entity, "entity");
Category category = (Category) entity;
Database.DeleteAll(Database.StoryViewDataSource.Where(v => v.Story.CategoryId == category.Id));
Database.DeleteAll(Database.CommentSubscribtionDataSource.Where(cs => cs.Story.CategoryId == category.Id));
Database.DeleteAll(Database.CommentDataSource.Where(c => c.Story.CategoryId == category.Id));
Database.DeleteAll(Database.VoteDataSource.Where(v => v.Story.CategoryId == category.Id));
Database.DeleteAll(Database.MarkAsSpamDataSource.Where(sp => sp.Story.CategoryId == category.Id));
Database.DeleteAll(Database.StoryTagDataSource.Where(st => st.Story.CategoryId == category.Id));
Database.DeleteAll(Database.StoryDataSource.Where(s => s.CategoryId == category.Id));
base.Remove(category);
}
}
}
StoryRepository
namespace Kigg.Repository.LinqToSql
{
//using statements omitted
public class StoryRepository : BaseRepository<IStory, Story>, IStoryRepository
{
//code omitted
public override void Remove(IStory entity)
{
Check.Argument.IsNotNull(entity, "entity");
Story story = (Story) entity;
Database.DeleteAll(Database.StoryViewDataSource.Where(sv => sv.StoryId == story.Id));
Database.DeleteAll(Database.CommentSubscribtionDataSource.Where(cs => cs.StoryId == story.Id));
Database.DeleteAll(Database.CommentDataSource.Where(c => c.StoryId == story.Id));
Database.DeleteAll(Database.VoteDataSource.Where(v => v.StoryId == story.Id));
Database.DeleteAll(Database.MarkAsSpamDataSource.Where(sp => sp.StoryId == story.Id));
Database.DeleteAll(Database.StoryTagDataSource.Where(st => st.StoryId == story.Id));
base.Remove(story);
}
}
}
Would I be correct in assuming that a better design would have the CategoryRepository calling the Remove method on the StoryRepository, thereby delegating the concern for the Story's child object removal to the StoryRepository where it belongs? From a maintenance point-of-view any additions to the Story's children would require DeleteAll calls to be added to both the CategoryRepository and the StoryRepository.
What would be a better implementation?
Should the CategoryRepository be refactored to use the StoryRepository directly?:
CategoryRepository (refactor)
namespace Kigg.Repository.LinqToSql.CategoryRepository{
//using statements omitted
public class CategoryRepository : BaseRepository<ICategory, Category>, ICategoryRepository
{
//code omitted
public override void Remove(ICategory entity)
{
Check.Argument.IsNotNull(entity, "entity");
Category category = (Category) entity;
// refactor - start
StoryRepository _storyRepository = new StoryRepository( Database );
category.Stories.ForEach( story => _storyRepository.Remove( story ) );
// refactor - end
base.Remove(category);
}
}
}
This refactor would allow the CategoryRepository to reuse the removal logic in the StoryRepository and should also reuse the same LinqToSql DataContext that is referenced by the Database argument given to the StoryRepository constructor. But when it comes to unit testing it starts to have an odor.
Would a better refactor include using IoC (Kigg uses Unity as its Ioc container) to inject the PerWebRequest scoped instance of IStoryRepository into the CategoryRepository's constructor?
CategoryRepository (refactor take 2)
namespace Kigg.Repository.LinqToSql.CategoryRepository{
//using statements omitted
public class CategoryRepository : BaseRepository<ICategory, Category>, ICategoryRepository
{
private readonly IStoryRepository _storyRepository;
public CategoryRepository(IDatabase database, IStoryRepository storyRepository) : base(database)
{
Check.Argument.IsNotNull(storyRepository, "storyRepository");
_storyRepository = storyRepository;
}
public CategoryRepository(IDatabaseFactory factory, IStoryRepository storyRepository) : base(factory)
{
Check.Argument.IsNotNull(storyRepository, "storyRepository");
_storyRepository = storyRepository;
}
//code omitted
public override void Remove(ICategory entity)
{
{
Check.Argument.IsNotNull(entity, "entity");
Category category = (Category) entity;
// refactor - start
category.Stories.ForEach( story => _storyRepository.Remove( story ) );
// refactor - end
base.Remove(category);
}
}
}
With this second refactor we can now inject an instance of IStoryRepository into CategoryRepository during unit testing and via Unity Ioc. Of course we would have to extend this refactor to each repository class so they could tend to their own children's responsibilites.
What are everyone's thoughts?
