Convention for read/write methods with Entity Framework. Controller vs DbContext?

Question

I'm writing a lot of basic CRUD views and controller logic for a website I'm building. So far I've written most of my code in the controllers, this includes the usual validation, input-sanitation, and error handling. Should I be writing all my DB I/O code in my controllers? Or should I move some over to my DbContext? I ask this, because I've heard conflicting views on how much should be going on inside controller classes, versus model classes? And is it appropriate to pass an instance of the Db context out of the controller? Or should I maybe do it with extension classes, on the DbContext?

For Example:

public ActionResult Create(ThingViewModel vModel)
{
    try
    {
        if (ModelState.IsValid)
        {
            var nm = vModel.ToActualModel();

            nm.RelatedThing = nm.RelatedThingId == null ? 
                null : db.RelatedThings.Single(v => v.Id == nm.RelatedThingId);
            nm.UtcCreatedOn = DateTime.UtcNow;

            db.Thing.Add(nm);
            db.SaveChanges();

            var successMessage = "You have created a new Thing!";
            return RedirectToAction("Index", new { successMessage = successMessage });
        }
        else
        {
            ViewBag.EntityName = "Thing";
            ViewBag.ControllerName = "Thing";
            ViewBag.Title = "Admin | Thing - Create";

            return View("~/Views/Thing/Create.cshtml", vModel);
        }
    }
    catch(Exception e)
    {
        var errorMessage = "An error occured when creating a new thing!";
        return RedirectToAction("Index", new { errorMessage = errorMessage });
    }
}

Should this become:

public ActionResult Create(ThingViewModel vModel)
{
    try
    {
        if (ModelState.IsValid)
        {
            db.CreateNewThing(vModel) // Defined elsewhere

            var successMessage = "You have created a new Thing!";
            return RedirectToAction("Index", new { successMessage = successMessage });
        }
        else
        {
            ViewBag.EntityName = "Thing";
            ViewBag.ControllerName = "Thing";
            ViewBag.Title = "Admin | Thing - Create";

            return View("~/Views/Thing/Create.cshtml", vModel);
        }
    }
    catch(Exception e)
    {
        var errorMessage = "An error occured when creating a new thing!";
        return RedirectToAction("Index", new { errorMessage = errorMessage });
    }
}

Inside DbContext:

 public bool CreateNewThing(ThingViewModel vModel)
 {
     //Thing Creation logic
     Things.Add(thing);
     SaveChanges();
 }

For clarification, I want to write my Create/Edit/Delete logic once for each entity, and be able to use them in other controllers. So if I have Person and Pet entities, along with a PersonController and PetController, there are times where a PersonViewModel containing a List<PetViewModel> needs to be written to the database: a Person needs to be created along with each Pet. However, PetController already has public ActionResult Create(PetViewModel vm) defined, but I can't use that from inside the PersonController for just writing a new Pet to the database. So, I would like to move the db logic of Create(PetViewModel vm) somewhere else, where I can access it from inside other controllers. Where would I move it? And is it appropriate to pass a reference to my DbContext from a controller to a static helper method public static bool CreateHelper(DbContext db, PetViewModel vm)?

Definitely don't abstract to Things.Add(thing);, that's called repository pattern and it is viewed by the sane as an anti-pattern. — Inspector Squirrel
– Inspector Squirrel, Commented Mar 3, 2015 at 15:40
Things.Add(thing) is already defined in the DbContext class, I wouldn't be writing it. Are you familiar with the EntityFramework ORM, because I'm pretty sure it inherently implements the Repository pattern. — wtyneb
– wtyneb, Commented Mar 3, 2015 at 15:45
Yes. It does, which is why it makes zero sense to implement your own repository on top of it, sorry, in my first comment I meant to say it's an anti-pattern to abstract EF into a repository. — Inspector Squirrel
– Inspector Squirrel, Commented Mar 3, 2015 at 15:46
Makes sense, then from the sounds of it your confirming my original hunch to keep the helpers out of my DAL. But I'm wondering whether I should put them in their associated controllers, or as separate helper classes. — wtyneb
– wtyneb, Commented Mar 3, 2015 at 15:49
I have a link to something that one of the guys in the C# chatroom wrote, which is basically a small wrapper around EF that gives you some control over which collections are accessible. You can take a look at it here. It's a Unit of Work implementation, if you've heard of that before, it works well with EF. — Inspector Squirrel
– Inspector Squirrel, Commented Mar 3, 2015 at 15:52

brianc · Accepted Answer · 2015-03-03 15:51:03Z

2

The first one is correct. Remember it is an MVC pattern - model - view - controller. Your controller is doing exactly what it should including retrieving information from the DB using the dbcontext (which in turn is doing exactly what it should be doing). It still follows good MVC practice of separating concerns but not at the expense of over complicating things. I learned this the hard way a few years back when I felt it was more important to be "correct" than use my common sense. Putting CreateNewThing in the db context gives you no real benefit and in larger more complex applications just muddies the waters.

answered Mar 3, 2015 at 15:51

brianc

4752 silver badges23 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

wtyneb Over a year ago

But right now, I have some duplicated Create logic when modifying related entities from inside one controller. I added some clarification at the bottom of my original question.

brianc Over a year ago

If you need to treat Persons and Pets as a combination on a regular basis you may want to consider refactoring your app. Once again, direct model to DB table mapping is all fine as long as it serves the purpose of your application. It seems to me that a more complex model that is a combination of Persons and Pets (PeopleAndPets) would contain both and a single PeoplAndPets controller would handle each plus mixes of both. this is one of the judgement calls that needs to be made based on what the purpose of the app is and most common usage scenarios.

Pharylon · Accepted Answer · 2015-03-03 15:45:22Z

2

I think it's fine - and good - to create helper classes in the MVC Project that aren't necessarily controllers, but it shouldn't be mixed with your datalayer. db.CreateNewThing(vModel) should be avoided. The db is for data storage and retrieval, not for converting models or doing anything like that.

answered Mar 3, 2015 at 15:45

Pharylon

9,9763 gold badges38 silver badges61 bronze badges

Comments

Community · Accepted Answer · 2017-05-23 11:50:52Z

One pattern you can utilize is the command pattern. This way you can separate the business logic from the controller.

See also: Command, CommandHandler and CommandInvoker

Boilerplate:

public interface ICommandResponse
{
    bool Success { get; }
    IReadOnlyCollection<string> Errors { get; }
}

public class CommandResponse : ICommandResponse
{
    public bool Success { get; set; }
    public IReadOnlyCollection<string> Errors { get; set; }
}

public interface ICommand<in TModel>
{
    ICommandResponse Handle(TModel model);
}

public abstract class CommandBase<TModel> : ICommand<TModel>
{
    public abstract ICommandResponse Handle(TModel model);

    protected ICommandResponse Success()
    {
        return new CommandResponse { Success = true };
    }

    protected ICommandResponse Fail(params string[] errors)
    {
        return new CommandResponse { Errors = new ReadOnlyCollection<string>(errors) };
    }
}

Code:

public class CreateThingCommand : CommandBase<ThingViewModel>
{
    private readonly DbContext _context;

    public CreateThingCommand(DbContext context)
    {
        _context = context;
    }

    public override ICommandResponse Handle(ThingViewModel viewModel)
    {
        var model = viewModel.ToActualModel();

        model.RelatedThing =
            model.RelatedThingId == null
            ? null
            : _context.RelatedThings.Single(v => v.Id == model.RelatedThingId);
        model.UtcCreatedOn = DateTime.UtcNow;

        _context.Thing.Add(model);
        _context.SaveChanges();

        return Success();
    }
}

// Inside controller
public ActionResult Create(ThingViewModel vModel)
{
    try
    {
        if (ModelState.IsValid)
        {
            var result = createThingCommand.Handle(vModel);

            if(result.Success)
            {
                var successMessage = "You have created a new Thing!";
                return RedirectToAction("Index", new { successMessage = successMessage });
            }

            // Handle error
            return RedirectToAction("Index", new { failMessage = "Something went wrong" });
        }

        ViewBag.EntityName = "Thing";
        ViewBag.ControllerName = "Thing";
        ViewBag.Title = "Admin | Thing - Create";

        return View("~/Views/Thing/Create.cshtml", vModel);
    }
    catch(Exception e)
    {
        var errorMessage = "An error occured when creating a new thing!";
        return RedirectToAction("Index", new { errorMessage = errorMessage });
    }
}

Collectives™ on Stack Overflow

Convention for read/write methods with Entity Framework. Controller vs DbContext?

3 Answers 3

2 Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related