Saving data in MVC

Question

We have a lot of Controller ActionResults like this in our MVC project. The controllers get really cluttered up. This is a simple example, but we have much more complex ActionResults.

This controller accepts a model, and if the primary key is Guid.Empty, handles it as an Add/Create. Otherwise it handles it as an edit.

When we started this project we were told that we don't need a Data Layer, since Entity Framework handles a lot of things, but I'm thinking it may be better to have one to handle some of these.

[HttpPost]
[ValidateAntiForgeryToken]
public virtual async Task<ActionResult> _projectedit(ProjectCreate model)
{
 if (ModelState.IsValid)
 {
 Project project = null;
 if (model.ProjectId == Guid.Empty)
 {
 project = new Project()
 {
 ProjectTypeId = model.ProjectTypeId,
 ProjectTemplateId = model.ProjectTemplateId,
 Title = model.Title,
 Subtitle = model.Subtitle,
 CoverImage = model.CoverImage,
 Volume = model.Volume,
 Issue = model.Issue,
 Edition = model.Edition,
 CopyrightYear = model.CopyrightYear,
 WordCount = model.WordCount,
 ProjectDescription = model.ProjectDescription,
 CreatedById = SessionUser.ProfileId,
 IsActive = true
 };
 db.Project.Add(project);
 }
 else
 {
 project = db.Project.Find(model.ProjectId);
 project.ProjectTypeId = model.ProjectTypeId;
 project.ProjectTemplateId = model.ProjectTemplateId;
 project.Title = model.Title;
 project.Subtitle = model.Subtitle;
 project.CoverImage = model.CoverImage;
 project.Volume = model.Volume;
 project.Issue = model.Issue;
 project.Edition = model.Edition;
 project.CopyrightYear = model.CopyrightYear;
 project.WordCount = model.WordCount;
 project.ProjectDescription = model.ProjectDescription;
 }
 db.SaveChanges();
 var success = new StringBuilder();
 success.Append("<p>");
 success.Append(Tools.Format.Title(model.Title, model.Subtitle));
 success.Append(" has been created and can be accessed from the <a href=\"/project/\">Projects</a> page");
 success.Append(".</p>");
 success.Append("<p><a href=\"/\">Return to home page</a>.");
 AddMessage(success.ToString());
 return RedirectToAction("Edit", "Project", new { id = project.ProjectId });
 }
 return View(model);
}

• Mark Kenyon: Btw, this code contains pieces from more than one developer, so this will help our whole team. Thank you.
• Afshar: It maybe useful to use something like AutoMapper to assign properties so reduce code lines.

Answer 1

1. Extract that logic into a data layer. Minimally, extract a private Upsert method.
2. Create an extension method for cloning the properties from one of your models to another.
3. Stop this nonsense.

success.Append("<p>");
success.Append(Tools.Format.Title(model.Title, model.Subtitle));
success.Append(" has been created and can be accessed from the <a href=\"/project/\">Projects</a> page");
success.Append(".</p>");
success.Append("<p><a href=\"/\">Return to home page</a>.");
AddMessage(success.ToString());

The view logic belongs in the view. Create a partial view that gets conditionally rendered. (Toggled on/off)

• RubberDuck: I just noticed your note about being told you didn't need a data layer because you're using entity framework. I don't know where people ever got that idea, but it's common and wrong in my opinion.
• Mark Kenyon: RubberDuck, are there any samples out there showing a good design pattern for the data layer? We have our Models/Context in it's own project. ViewModels are in the Web project.

Answer 2

public virtual async Task<ActionResult> _projectedit(ProjectCreate model) 

• Based on the Task-based Asynchronous Pattern (TAP) one should add the postfix Async to the methodname for async methods.
• Based on the NET naming guidelines methods should be named using PascalCase casing. Having an underscore prefixed methodname is really unusual. Underscore prefixing is tolerable for private class level variables but thats it.
• Based on the same naming guideline method names should be made out of verbs or verb phrases so if you want to keep edit and project in that name it should be EditProject.
• This method is either adding or updating a Project so a name which is reflecting this would be better like AddOrUpdateProject.
• Extracting the adding and updating to separate methods will reduce the length of that method and makes it more readable.
• The message you are building is lying if a Project is edited, because it states " has been created". Best way to do the messagebuilding is, if you want/have to go this way, to extract this to a separate method passing the ProjectCreate model as an argument.
• By reverting the condition if (ModelState.IsValid) you could (and should) return early which shaves off one level of indentation which results in less horizontal spacing hence increasing readability.
• A method named AddMessage doesn't tell where the message is added to which will reduce the ability of Sam the maintainer to graps the purpose at first glance.
• Having the async keyword without having an await will result in the method running synchronously and a compiler warning will be returned.

Implementing some of this points will lead to

[HttpPost]
[ValidateAntiForgeryToken]
public virtual async Task<ActionResult> AddOrUpdateProjectAsync(ProjectCreate model)
{
 if (!ModelState.IsValid)
 {
 return View(model);
 }
 Guid projectId = Guid.Empty; 
 if (model.ProjectId == Guid.Empty)
 { 
 projectId = AddProject(model);
 }
 else
 {
 projectId = UpdateProject(model);
 } 
 db.SaveChanges();
 AddMessage(model);
 return RedirectToAction("Edit", "Project", new { id = projectId });
}

• Mark Kenyon: Excellent, thank you. One question, AddProject and UpdateProject. I understand those methods are where a bunch of logic get put. But where should I put those? Should I have a DataLayer for MVC projects?
• Heslacher: This methods can be private ones which could live in the same class as the former one.