Help improving (refactoring) my code. Automapper - EF - asp.net mvc-3

Question

I have this 4 models - 2 Domain Models and 2 DTOs

public class Project
    {
        public int ID { get; set; }
        public string Name { get; set; }
        public virtual ICollection<Task> Tasks { get; set; }
    }

    public class Task
    {
        public int ID { get; set; }
        public virtual int ProjectID { get; set; }
        public string Name { get; set; }
        public virtual Project Project { get; set; }
    }

    public class ProjectDTO
    {
        [Required]
        public string Name { get; set; }
        public List<TaskDTO> Tasks { get; set; }
    }

    public class TaskDTO
    {
        [Required]
        public string Name { get; set; }
        public int ID { get; set; }
        public bool MarkRemove { get; set; }
    }

Heres my automapper configuration

Mapper.CreateMap<Project, ProjectDTO>();
Mapper.CreateMap<ProjectDTO, Project>().ForMember(p =>p.ID, opt=>opt.Ignore()).ForMember(p=>p.Tasks, opt=>opt.Ignore());
Mapper.CreateMap<Task, TaskDTO>();
Mapper.CreateMap<TaskDTO, Task>().ForMember(task=>task.ProjectID, opt=>opt.Ignore()).ForMember(task=>task.Project, opt=>opt.Ignore());

Heres my HttpPost Edit action

[HttpPost]
        public ActionResult Edit(int id, ProjectDTO p)
        {
            if (ModelState.IsValid)
            {
                var dbProject = db.Projects.Where(pr => pr.ID == id).Single();
                Mapper.Map(p, dbProject);
                foreach (var task in p.Tasks)
                {
                    Task dbTask;
                    try
                    {
                        dbTask = dbProject.Tasks.Where(t => t.ID == task.ID).Single();
                    }
                    catch
                    {
                        dbTask = new Task();
                        Mapper.Map(task, dbTask);
                        dbProject.Tasks.Add(dbTask);
                    }
                    if (task.MarkRemove)
                    {
                        db.Tasks.Remove(dbTask);
                    }
                    else {
                        Mapper.Map(task, dbTask);                    
                    }                  
                }
                db.Entry(dbProject).State = EntityState.Modified;
                db.SaveChanges();                
                TempData["Success"] = "Modelo Valido";
                return RedirectToAction("Index");
            }
            return View(p);
        }

Im not completely happy with this but I dont think there is a much cleaner approach to handling this somewhat complex scenario....

now that it is working I would like to at least refactor this to use repository pattern or something in a way that the controller action is not that convoluted.. this will eventually be production code :s

can anyone give me some advice on how to refactor this? please help.

Darin Dimitrov · Accepted Answer · 2011-07-26 06:59:15Z

1

I would use a service layer, like this:

public interface IProjectsService
{
    void RemoveTasks(int projectId, IEnumerable<int> taskIdsToRemove);
}

and then the controller would depend on this service layer:

public class ProjectsController : Controller
{
    private readonly IProjectsService _service;
    public ProjectsController(IProjectsService service)
    {
        _service = service;
    }

    public ActionResult Edit(int id)
    {
        // TODO: Add methods to your service layer
        // allowing to retrieve projects, then map
        // the resulting project into a view model
        throw new NotImplementedException();
    }

    [HttpPost]
    public ActionResult Edit(int id, ProjectDTO p)
    {
        if (!ModelState.IsValid)
        {
            return View(p);
        }

        var taskIdsToRemove = p.Tasks.Where(x => x.MarkRemove).Select(x => x.ID);
        _service.RemoveTasks(id, taskIdsToRemove);
        TempData["Success"] = "Modelo Valido";
        return RedirectToAction("Index");
    }
}

This way the controller logic is more weakly coupled to the way we do data access. That's an implementation detail that a controller should never have to worry about.

As a further improvement to the RemoveTasks method you could make it return a boolean indicate the success or failure of the operation along with an error message so that the Edit action could redisplay the view and show the error in case something goes wrong.

Now as far as this service layer is concerned, the RemoveTasks method is a business operation that could be built upon multiple CRUD operations with some repository. So this service layer would itself depend on a repository. It is only this repository that will have to know about EF or whatever you are using to do your data access.

So basically everytime I see someone asking a question about ASP.NET MVC and EF at the same time, for me, those are two completely different questions. ASP.NET MVC should not know anything about EF. EF should be buried away behind an abstraction of a repository.

answered Jul 26, 2011 at 6:59

Darin Dimitrov

1.0m276 gold badges3.3k silver badges3k bronze badges

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

4 Comments

nacho10f Over a year ago

Thanks for that very thorough answer..the only thing is that Im not only RemovingTasks. I am also adding and updating them...you would do this in the same method?

Darin Dimitrov Over a year ago

@NachoF, indeed. I didn't pay attention to this important detail. I would still do the entire operation inside a single service method (UpdateTasks for example) but in this case in addition to the list of ids of tasks to remove we need to pass a list of Task to update. This list of Task to update would be constructed using AutoMapper from the view model, exactly as you did in your example: var tasksToUpdate = new List<Task>(); Mapper.Map(p.Tasks, tasks);.

nacho10f Over a year ago

That is confusing cause then the MarkRemove attribute that I added in my DTO would get lost in the mapping... the UpdateTasks method would have no way of knowing which tasks to delete.

Darin Dimitrov Over a year ago

@NachoF, you would keep the taskIdsToRemove parameter to your service method so that it knows which tasks to delete.

Christopher Stieg · Accepted Answer · 2017-09-23 20:55:30Z

I realize this question has been inactive for a while, but I had the same problem, and though I would post my solution for anyone else struggling with the nested model not being saved during edit.

    [HttpPost]
    public ActionResult Edit(ProjectDTO p)
    {
        if (ModelState.IsValid)
        {
            // *****MODIFIED CODE HERE********
            for (int i = 0; i < p.Tasks.Count; i++) 
            {
                    db.Entry(p.Tasks[i]).State = EntityState.Modified;
            }

           // *************************************

            db.Entry(dbProject).State = EntityState.Modified;
            db.SaveChanges();                
            TempData["Success"] = "Modelo Valido";
            return RedirectToAction("Index");
        }
        return View(p);
    }

Basically, you want to set State of each nested model to Modified as well as the root model.

Collectives™ on Stack Overflow

Help improving (refactoring) my code. Automapper - EF - asp.net mvc-3

2 Answers 2

4 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related