Search code examples
c#htmlasp.net-mvcrefactoringdomain-object

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

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.

Solution

  • 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