8
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ Btw, this code contains pieces from more than one developer, so this will help our whole team. Thank you. \$\endgroup\$ – Mark Kenyon Dec 9 '15 at 13:34
  • \$\begingroup\$ It maybe useful to use something like AutoMapper to assign properties so reduce code lines. \$\endgroup\$ – Afshar Mohebbi Jul 26 '16 at 6:20
6
\$\begingroup\$
  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)

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – RubberDuck Dec 9 '15 at 12:29
  • \$\begingroup\$ 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. \$\endgroup\$ – Mark Kenyon Dec 9 '15 at 13:39
4
\$\begingroup\$
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 });

}
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Mark Kenyon Dec 9 '15 at 13:33
  • \$\begingroup\$ This methods can be private ones which could live in the same class as the former one. \$\endgroup\$ – Heslacher Dec 9 '15 at 13:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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