More ASP.NET MVC Best Practices

In this post, I’ll share some of the best practices and guidelines which I have come across while developing ASP.NET MVC web applications. I will not cover all best practices that are available, instead add some specific things that have not been mentioned in any blog post out there.

Existing best practices can be found on Kazi Manzur Rashid’s blog and Simone Chiaretta’s blog:

After reading the best practices above, read the following best practices.

kick it on DotNetKicks.com

Use model binders where possible

I assume you are familiar with the concept of model binders. If not, here’s a quick model binder 101: instead of having to write action methods like this (or a variant using FormCollection form[“xxxx”]):

[code:c#]

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Save()
{
    // ...

    Person newPerson = new Person();
    newPerson.Name = Request["Name"];
    newPerson.Email = Request["Email"];

    // ...
}

[/code]

You can now write action methods like this:

[code:c#]

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Save(FormCollection form)
{
    // ...

    Person newPerson = new Person();
    if (this.TryUpdateModel(newPerson, form.ToValueProvider()))
    {
        // ...
    }

    // ...
}

[/code]

Or even cleaner:

[code:c#]

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Save(Person newPerson)
{
    // ...
}

[/code]

What’s the point of writing action methods using model binders?

  • Your code is cleaner and less error-prone
  • They are LOTS easier to test (just test and pass in a Person)

Be careful when using model binders

I know, I’ve just said you should use model binders. And now, I still say it, except with a disclaimer: use them wisely! The model binders are extremely powerful, but they can cause severe damage…

Let’s say we have a Person class that has an Id property. Someone posts data to your ASP.NET MVC application and tries to hurt you: that someone also posts an Id form field! Using the following code, you are screwed…

[code:c#]

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Save(Person newPerson)
{
    // ...
}

[/code]

Instead, use blacklisting or whitelisting of properties that should be bound where appropriate:

[code:c#]

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Save([Bind(Prefix=””, Exclude=”Id”)] Person newPerson)
{
    // ...
}

[/code]

Or whitelisted (safer, but harder to maintain):

[code:c#]

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Save([Bind(Prefix=””, Include=”Name,Email”)] Person newPerson)
{
    // ...
}

[/code]

Yes, that can be ugly code. But…

  • Not being careful may cause harm
  • Setting blacklists or whitelists can help you sleep in peace

Never re-invent the wheel

Never reinvent the wheel. Want to use an IoC container (like Unity or Spring)? Use the controller factories that are available in MvcContrib. Need validation? Check xVal. Need sitemaps? Check MvcSiteMap.

Point is: reinventing the wheel will slow you down if you just need basic functionality. On top of that, it will cause you headaches when something is wrong in your own code. Note that creating your own wheel can be the better option when you need something that would otherwise be hard to achieve with existing projects. This is not a hard guideline, you’ll have to find the right balance between custom code and existing projects for every application you’ll build.

Avoid writing decisions in your view

Well, the title says it all.. Don’t do this in your view:

[code:c#]

<% if (ViewData.Model.User.IsLoggedIn()) { %>
  <p>...</p>
<% } else { %>
  <p>...</p>
<% } %>

[/code]

Instead, do this in your controller:

[code:c#]

public ActionResult Index()
{
    // ...

    if (myModel.User.IsLoggedIn())
    {
        return View("LoggedIn");
    }
    return View("NotLoggedIn");
}

[/code]

Ok, the first example I gave is not that bad if it only contains one paragraph… But if there are many paragraphs and huge snippets of HTML and ASP.NET syntax involved, then use the second approach. Really, it can be a PITA when having to deal with large chunks of data in an if-then-else structure.

Another option would be to create a HtmlHelper extension method that renders partial view X when condition is true, and partial view Y when condition is false. But still, having this logic in the controller is the best approach.

Don’t do lazy loading in your ViewData

I’ve seen this one often, mostly by people using Linq to SQL or Linq to Entities. Sure, you can do lazy loading of a person’s orders:

[code:c#]

<%=Model.Orders.Count()%>

[/code]

This Count() method will go to your database if model is something that came out of a Linq to SQL data context… Instead of doing this, retrieve any value you will need on your view within the controller and create a model appropriate for this.

[code:c#]

public ActionResult Index()
{
    // ...

    var p = ...;

    var myModel = new {
        Person = p,
        OrderCount = p.Orders.Count()
    };
    return View(myModel);
}

[/code]

Note: This one is really for illustration purpose only. Point is not to pass the datacontext-bound IQueryable to your view but instead pass a List or similar.

And the view for that:

[code:c#]

<%=Model.OrderCount%>

[/code]

Motivation for this is:

  • Accessing your data store in a view means you are actually breaking the MVC design pattern.
  • If you don't care about the above: when you are using a Linq to SQL datacontext, for example, and you've already closed that in your controller, your view will error if you try to access your data store.

Put your controllers on a diet

Controllers should be really thin: they only accept an incoming request, dispatch an action to a service- or business layer and eventually respond to the incoming request with the result from service- or business layer, nicely wrapped and translated in a simple view model object.

In short: don’t put business logic in your controller!

Compile your views

Yes, you can do that. Compile your views for any release build you are trying to do. This will make sure everything compiles nicely and your users don’t see an “Error 500” when accessing a view. Of course, errors can still happen, but at least, it will not be the view’s fault anymore.

Here’s how you compile your views:

1. Open the project file in a text editor. For example, start Notepad and open the project file for your ASP.NET MVC application (that is, MyMvcApplication.csproj).

2. Find the top-most <PropertyGroup> element and add a new element <MvcBuildViews>:

[code:c#]

<PropertyGroup>

...
<MvcBuildViews>true</MvcBuildViews>

</PropertyGroup>

[/code]

3. Scroll down to the end of the file and uncomment the <Target Name="AfterBuild"> element. Update its contents to match the following:

[code:c#]

<Target Name="AfterBuild" Condition="'$(MvcBuildViews)'=='true'">

<AspNetCompiler VirtualPath="temp"
PhysicalPath="$(ProjectDir)\..\$(ProjectName)" />
</Target>

[/code]

4. Save the file and reload the project in Visual Studio.

Enabling view compilation may add some extra time to the build process. It is recommended not to enable this during development as a lot of compilation is typically involved during the development process.

More best practices

There are some more best practices over at LosTechies.com. These are all a bit advanced and may cause performance issues on larger projects. Interesting read but do use them with care.

kick it on DotNetKicks.com

This is an imported post. It was imported from my old blog using an automated tool and may contain formatting errors and/or broken images.

Leave a Comment

avatar

14 responses

  1. Avatar for geoffrey emery
    geoffrey emery May 6th, 2009

    Nice Post!

  2. Avatar for John
    John May 7th, 2009

    If you're excluding the Id from the Model, how are you locating the person in the database? In the case of save that works, in the case of Update it doesn't.

  3. Avatar for CarlH
    CarlH May 7th, 2009

    Great post but you didn't motivate "Don’t do lazy loading in your ViewData".

  4. Avatar for maartenba
    maartenba May 7th, 2009

    True for updates. But you get the idea.

    For example,you don't want people ediitng your UnitsInStock or SalesPrice fields when doing a form post somewhere.

  5. Avatar for maartenba
    maartenba May 7th, 2009

    Will add that to the post in a second. Thanks for noticing!

  6. Avatar for Melayu Boleh
    Melayu Boleh May 8th, 2009

    Great post but you didn't motivate "Don’t do lazy

  7. Avatar for GuyIncognito
    GuyIncognito May 11th, 2009

    I just purchased a PDF version of your new book from Packt Publishing. I think I've probably covered most of the material in the other ASP.NET MVC book I just recently finished reading, but since your blog is such a great source of continuing MVC information, I felt I had to show some support!

    Thanks and keep up the posts!

    ps. I also like the glasses. :)

  8. Avatar for maartenba
    maartenba May 12th, 2009

    Thanks for the support! :-)

  9. Avatar for MotoWilliams
    MotoWilliams May 30th, 2009

    [b]"Use model binders where possible"[/b] you might show using marker interfaces to also control what fields are able to be bound to.

  10. Avatar for Andrei Rînea
    Andrei Rînea June 9th, 2009

    I don't get how you passed an anonymous type instance to the view in the controller and used in the view directly. I have tried and, just as I thought, it doesn't compile..

  11. Avatar for maartenba
    maartenba June 9th, 2009

    That one is really for illustration purpose only. Point is not to pass the datacontext-bound IQueryable to your view but instead pass a List or similar.

  12. Avatar for Paul Kohler
    Paul Kohler June 11th, 2009

    ...more good tips - good to see the promotion of proper use of the ViewData, makes the controller more testable too...

  13. Avatar for simon
    simon August 20th, 2009

    Thanks for the guidelines, i have just started developing using mvc

  14. Avatar for Gleb
    Gleb November 18th, 2009

    Thanks for the post.
    May I ask a question - when you`re using white-listing or black-listing in model binding, what values will excluded properties/fields have in the view? Default value for its type? Or something else?