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.

Answer 1

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.

Answer 2

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.