What is the best way to refactor presentation code out of my domain objects in an ASP.NET MVC solution?

Question

I have just taken over an ASP.NET MVC project and some refactoring is required, but I wanted to get some thoughts / advice for best practices.

The site has an SQL Server backend and here is a review of the projects inside the solution:

• DomainObjects (one class per database table)
• DomainORM (mapping code from objects to DB)
• Models (business logic)
• MVC (regular ASP.NET MVC web setup) ---- Controllers ---- ViewModels ---- Views ---- Scripts

The first "issue" I see is that while the Domain objects classes are pretty much POCO with some extra "get" properties around calculated fields, there is some presentation code in the Domain Objects. For example, inside the DomainObjects project, there is a Person object and I see this property on that class:

 public class Person
 {

    public virtual string NameIdHTML
    {
        get
        {
           return "<a href='/People/Detail/" + Id + "'>" + Name + "</a> (" + Id + ")";
        }
    }
 }

so obviously having HTML-generated content inside the domain object seems wrong.

Refactor Approaches:

1. My first instinct was to move this to the ViewModel class inside the MVC project, but I see that there are a lot of views that hit this code so I don't want to duplicate code in each view model.

2. The second idea was to create PersonHTML class that was either:

   2a. A wrapper that took in a Person in the constructor or

   2b. A class that inherited from Person and then has all of these HTML rendering methods.

   The view Model would convert any Person object to a PersonHTML object and use that for all rendering code.

I just wanted to see:

1. If there is a best practice here as it seems like this is a common problem / pattern that comes up

2. How bad is this current state considered because besides feeling wrong, it is not really causing any major problems understanding the code or creating any bad dependencies. Any arguments to help describe why leaving code in this state is bad from a real practical sense (vs. a theoretical separation of concerns argument) would be helpful as well as there is debate in the team whether it's worth it to change.

Answer 1

I like TBD's comment. It's wrong because you are mixing domain concerns with UI concerns. This causes a coupling that you could avoid.

As for your suggested solutions, I don't really like any of them.

1. Introducing a view model. Yes, we should use view models, but we don't want to pollute them with HTML code. So an example of using a view would be if you've got a parent object, person type, and you want to show the person type on the screen. You would fill the view model with the person type name and not a full person type object because you only need the person type name on the screen. Or if your domain model had first and last name separate, but your view calls for FullName, you would populate the view model's FullName and return that to the view.

2. PersonHtml class. I'm not even sure what that would do. Views are what represent the HTML in an ASP.NET MVC application. You've got two options here:

   a. You could create a display template for you model. Here's a link to a Stack Overflow question to display templates, How to make display template in MVC 4 project

   b. You could also write a HtmlHelper method that would generate the correct HTML for you. Something like @Html.DisplayNameLink(...) Those would be your best options. Here's a link for understanding HtmlHelpers https://download.microsoft.com/download/1/1/f/11f721aa-d749-4ed7-bb89-a681b68894e6/ASPNET_MVC_Tutorial_9_CS.pdf

Answer 2

I've wrestled with this myself. When I had code in views that were more logic based than HTML, I created an enhanced version of the HtmlBuilder. I extended certain domain objects to automatically print out this helper, with it's contents based off of domain functions, that could then just be printed onto a view. However, the code becomes very cluttered and unreadable (especially when your trying to figure out where it came from); for these reasons, I suggest removing as much presentation/view logic from the domain as possible.

However, after that I decided to take another look at Display and Editor Templates. And I've come to appreciate them more, especially when combined with T4MVC, FluentValidation, and custom Metadata Providers, among other things. I've found using HtmlHelpers and extending the metadata or routing table to much more cleaner way of doing things, but you also start playing with systems that are less documented. However, this case is relatively simple.

So, first off, I would ensure you have a route defined for that entity, which is looks like you would with the default MVC route, so you can simply do this in a view:

//somewhere in the view, set the values to the desired value for the person you have
@{
    var id = 10; //random id
    var name = "random name";
}
//later:
<a href="@Url.Action("People", "Detail", new { id = id })"> @name  ( @id )</a>

Or, with T4MVC:

<a href="@Url.Action(MVC.People.Detail(id))"> @name ( @id )</a>

This means, with regards to the views/viewmodels, the only dependency these have is the id and name of the Person, which I would presume your existing view models ought to have (removing that ugly var id = x from above):

<a href="@Url.Action("People", "Detail", new { id = Model.PersonId } )"> 
    @Model.Name ( @Model.PersonId )
</a>

Or, with T4MVC:

<a href="@Url.Action( MVC.People.Detail( Model.PersonId ) )"> 
    @Model.Name ( @Model.PersonId )
</a>

Now, as you said, several views consume this code, so you would need to change the views to conform to the above. There are other ways do it, but every suggestion I have would require changing the views, and I believe this is the cleanest way. This also has a feature of using the route table, meaning that if the routing system is updated, then the updated url would print out here without worries, as opposed to hard coding it in the domain object as a url (that is dependent on the route system to have been set up in a specific manner for that url to work).

One of the my other suggestions would be to build a Html Helper, called Html.LinkFor( c => model ) or something like that, but, unless if you want it to dynamically determine the controller/action based off the type, that is kind of unnecessary.

Answer 3

How bad is this current state considered because besides feeling wrong, it not really causing any major problems understanding the code or creating any bad dependencies.

The current state is very bad, not only because UI code is included in domain code. That would be already pretty bad, but this is worse. The NameIdHTML property returns a hardcoded link to the person's UI page. Even in UI code you should not hardcode these kind of links. That is what LinkExtensions.ActionLink and UrlHelper.Action are for.

If you change your controller or your route, the link will change. LinkExtensions and UrlHelper are aware of this and you don't need any further changes. When you use a hardcoded link, you need to find all places in your code where such a link is hardcoded (and you need to be aware that those places exist). To make matters even worse, the code you need to change is in the business logic which is in the opposite direction of the chain of dependencies. This is a maintenance nightmare and a major source of errors. You need to change this.

If there is a best practice here as it seems like this is a common problem / pattern that comes up.

Yes, there is a best practice and that is using the mentioned LinkExtensions.ActionLink and UrlHelper.Action methods whenever you need a link to a page returned by a controller action. The bad news is that this means changes at multiple spots in your solution. The good news is that it's easy to find those spots: just remove the NameIdHTML property and the errors will pop up. Unless you are accessing the property by reflection. You will need to do a more careful code search in this case.

You will need to replace NameIdHTML by code that uses LinkExtensions.ActionLink or UrlHelper.Action to create the link. I assume that NameIdHTML returns HTML code that should be used whenever this person is to be shown on an HTML page. I also assume that this is a common pattern in your code. If my assumption is true, you can create a helper class that converts business objects to their HTML representations. You can add extension methods to that class that will provide the HTML representation of your objects. To make my point clear I assume (hypothetically), that you have a Department class that also has Name and Id and that has a similar HTML representation. You can then overload your conversion method:

public static class BusinessToHtmlHelper {
    public static MvcHtmlString FromBusinessObject( this HtmlHelper html, Person person) {
        string personLink = html.ActionLink(person.Name, "Detail", "People",
            new { id = person.Id }, null).ToHtmlString();
        return new MvcHtmlString(personLink + " (" + person.Id + ")");
    }

    public static MvcHtmlString FromBusinessObject( this HtmlHelper html,
        Department department) {

        string departmentLink = html.ActionLink(department.Name, "Detail", "Departments",
            new { id = department.Id }, null).ToHtmlString();
        return new MvcHtmlString(departmentLink + " (" + department.Id + ")");
    }
}

In your views you need to replace NameIdHTML by a call to this helper method. For example this code...

@person.NameIdHTML

...would need to be replaced by this:

@Html.FromBusinessObject(person)

That would also keep your views clean and if you decide to change the visual representation of Person you can easily change BusinessToHtmlHelper.FromBusinessObject without changing any views. Also, changes to your route or controllers will be automatically reflected by the generated links. And the UI logic remains with the UI code, while business code stays clean.

If you want to keep your code completely free from HTML, you can create a display template for your person. The advantage is that all your HTML is with the views, with the disadvantage of needing a display template for each type of HTML link you want to create. For Person the display template would look something like this:

@model Person

@Html.ActionLink(Model.Name, "Detail", "People", new { id = Model.Id }, null) ( @Html.DisplayFor(p => p.Id) )

You would have to replace your references to person.NameIdHTML by this code (assuming your model contains a Person property of type Person):

@Html.DisplayFor(m => m.Person)

You can also add display templates later. You can create BusinessToHtmlHelper first and as a second refactoring step in the future, you change the helper class after introducing display templates (like the one above):

public static class BusinessToHtmlHelper {
    public static MvcHtmlString FromBusinessObject<T>( this HtmlHelper<T> html, Person person) {
        return html.DisplayFor( m => person );
    }
    //...
}

If you were careful only to use links created by BusinessToHtmlHelper, there will be no further changes required to your views.