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

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.

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.