Simple CRUD application with n layered architecture

Question

I had a technical test with a simple CRUD application where I used n layered architecture as explained on the Patterns In Action book that I bought.

However after delivering one of their feedbacks was the following.

DbContext lifetime is completely wrong. (literally)

I will copy the relevant files on this question, because I want to learn what I did wrong and if that product I bought has just problems conceptually.

So, in my DataAccess class library I have this:

namespace DataObjects
{ 
 // abstract factory interface. Creates data access objects.
 // ** GoF Design Pattern: Factory.
 public interface IDaoFactory
 {
 //Product Dao interface that must be implemented by each provider
 IProductDao ProductDao { get; }
 //Color Dao interface that must be implemented by each provider
 IColorDao ColorDao { get; }
 //Size Dao interface that must be implemented by each provider
 ISizeDao SizeDao { get; }
 //Category Dao interface that must be implemented by each provider
 ICategoryDao CategoryDao { get; }
 //File Dao interface that must be implemented by each provider
 IFileDao FileDao { get; }
 //File Error Dao that must be implemented by each interface
 IFileErrorDao FileErrorDao { get; }
 }
}

Then I have also this Interface

using BusinessObjects;
using System.Collections.Generic;
namespace DataObjects
{
 public interface ICategoryDao
 {
 //Gets a list of categories
 List<Category> GetCategories();
 //Inserts one category 
 void InsertCategory(Category category);
 //To verify if category exists
 bool CategoryExists(string category);
 //Get Category by name
 Category GetCategoryByName(string category);
 }
}

And now, in the EntityFramework namespace I have the following implementations

namespace DataObjects.EntityFramework
{
 // Data access object factory
 // ** Factory Pattern
 public class DaoFactory : IDaoFactory
 {
 public IProductDao ProductDao => new ProductDao();
 public IColorDao ColorDao => new ColorDao();
 public ISizeDao SizeDao => new SizeDao();
 public ICategoryDao CategoryDao => new CategoryDao();
 public IFileDao FileDao => new FileDao();
 public IFileErrorDao FileErrorDao => new FileErrorDao();
 }
}

DaoCategory implementation

using AutoMapper;
using System.Collections.Generic;
using System.Linq;
using BusinessObjects;
namespace DataObjects.EntityFramework
{
 // Data access object for Product
 // ** DAO Pattern
 public class CategoryDao : ICategoryDao
 {
 /// <summary>
 /// Inserts category into database
 /// </summary>
 /// <param name="category"></param>
 public void InsertCategory(Category category)
 {
 using (var context = new ExamContext())
 {
 Mapper.Initialize(cfg => cfg.CreateMap<Category, CategoryEntity>());
 var entity = Mapper.Map<Category, CategoryEntity>(category);
 context.CategoryEntities.Add(entity);
 context.SaveChanges();
 // update business object with new id
 category.Id = entity.Id;
 }
 }
 /// <summary>
 /// Gets all categories from database
 /// </summary>
 /// <returns>Returns a list of Category</returns>
 public List<Category> GetCategories()
 {
 using (var context = new ExamContext())
 {
 Mapper.Initialize(cfg => cfg.CreateMap<CategoryEntity, Category>());
 var categories = context.CategoryEntities.ToList();
 return Mapper.Map<List<CategoryEntity>, List<Category>>(categories);
 }
 }
 /// <summary>
 /// Verifies if one category name exists
 /// </summary>
 /// <param name="category">Category name</param>
 /// <returns>Returns true if exists</returns>
 public bool CategoryExists(string category)
 {
 using (var context = new ExamContext())
 {
 return context.CategoryEntities.Any(x => x.CategoryName == category);
 }
 }
 /// <summary>
 /// Gets color by name
 /// </summary>
 /// <param name="categoryName">color name</param>
 /// <returns>Category</returns>
 public Category GetCategoryByName(string categoryName)
 {
 using (var context = new ExamContext())
 {
 Mapper.Initialize(cfg => cfg.CreateMap<CategoryEntity, Category>());
 var category = context.CategoryEntities.FirstOrDefault(x => x.CategoryName == categoryName);
 return Mapper.Map<CategoryEntity, Category>(category);
 }
 }
 }
}

• πάντα ῥεῖ: Improve your question title to sum up shortly what this code is siupposed to do please.
• πάντα ῥεῖ: Not much of an improvement so far, sorry.
• πάντα ῥεῖ: What about telling us about that Factory pattern implementation? Also isn't that product you bought meant to generate that stuff for you?
• t3chb0t: Is this an interview question?
• Luis Valencia: a technical test question.

Answer 1

The problem is that Entity Framework's DbContext is really a unit of work. By newing them up inside each method you lose the ability to do several interesting things in the same transaction/unit of work in an easy way. You should be able to have multiple repositories (Dao in your parlance) using the same context/unit of work.

Your code is also incredibly coupled to EF and you're violating the dependency inversion principle.

This is the best write up of DbContext lifetime that I've ever read: Managing DbContext the right way with Entity Framework 6: an in-depth guide. Although you're not actually using any of the 3 main patterns discussed.

If that Mapper is Automapper, that's also the wrong place to be configuring it and is a conflation of concerns.

• Luis Valencia: thank you, can you please ellaborate more about the Automapper? where it should be? I just based my code on patterns in action sample code from www.dofactory.com product I bought
• Luis Valencia: can you please also ellaborate more about the vioation of DI, and how to fix it, or a good sample about it
• RobH: @LuisValencia-MVP Automapper here: github.com/AutoMapper/AutoMapper/wiki/Configuration. Main point is to have the configuration happen once. Not every time you use it. Note also that the instance API is generally preferred over the static API too.

Answer 2

I don't see any reason for why the lifetime of the DbContext should be completely wrong. It's too general and probably just meant to not let you pass the question.

The only things that I'm not so fond of are

• the mapping between the EF entities and and DTOs - why would you want to do this? Don't you have more interesting code to write? ;-)
• the Dao suffix polution everywhere.
• I've never seen any real world application where something as trivial as CRUD only would work. You always have joins and other sophisticated inserts so it's nice only in theory. You also almost never need CRUD interfaces for every entity type becasue some of them cannot exist without some other ones and need to be inserted/deleted together.