4
\$\begingroup\$

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);
            }
        }
    }
}
\$\endgroup\$
14
  • 3
    \$\begingroup\$ Improve your question title to sum up shortly what this code is siupposed to do please. \$\endgroup\$ May 9, 2017 at 18:18
  • \$\begingroup\$ Not much of an improvement so far, sorry. \$\endgroup\$ May 9, 2017 at 18:22
  • 2
    \$\begingroup\$ What about telling us about that Factory pattern implementation? Also isn't that product you bought meant to generate that stuff for you? \$\endgroup\$ May 9, 2017 at 18:31
  • \$\begingroup\$ Is this an interview question? \$\endgroup\$
    – t3chb0t
    May 9, 2017 at 18:33
  • \$\begingroup\$ a technical test question. \$\endgroup\$ May 9, 2017 at 18:34

2 Answers 2

2
+50
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ 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 \$\endgroup\$ May 12, 2017 at 15:24
  • \$\begingroup\$ can you please also ellaborate more about the vioation of DI, and how to fix it, or a good sample about it \$\endgroup\$ May 12, 2017 at 16:05
  • \$\begingroup\$ @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. \$\endgroup\$
    – RobH
    May 13, 2017 at 6:36
2
\$\begingroup\$

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.
\$\endgroup\$

Your Answer

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

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