OPTEN, das einzige Umbraco-zertifizierte Unternehmen der Schweiz

Clean code: separation of concerns

The problem.

We have a webshop which various companies use. Many of the companies have special rules for the form of the addresses which their employees will enter when using the shop. For example some companies have a personnel number which they require the user to enter along with their address. Also depending on which page the user is on there may be different rules, for example if the address is shown in a modal popup, then the personnel number should not be shown. And then an employee can have multiple addresses, so another rule for some companies is that the personnel number should only be shown for the default address. Originally there were no rules for different companies, so the address view was simple, however over time it started to become more complicated and messy, here is an example:

@model Ch.Opten.Commerce.PresentationLayer.Interfaces.Models.Customer.IAddressVM
@if (Model.Workflow == Workflows.CompanyA
       || (Model.Workflow == Workflows.CompanyB && Model.isDefault))
{
    <div class="form-group">
        @Html.LabelRequired(Html.NameFor(model => model.PersonnelNumber).ToString(), null)
        @Html.TextBox(Html.NameFor(model => model.PersonnelNumber).ToString(),
                                 Model.PersonnelNumber)
        @Html.ValidationMessage(Html.NameFor(model => model.PersonnelNumber))
    </div>
}
else
{
    @Html.Hidden(Html.NameFor (model => model.PersonnelNumber).ToString(),
                           Model.PersonnelNumber)
}

The aim was to show the personnel number for the address IF this was for company A OR IF it was for company B AND this was the default address. In this situation the view must know about all the different rules for the different companies. This is acceptable if there are only two companies. But what about when there are companies A-Z with special rules for each one? The view will become horribly messy with all this business logic within the html markup. In my case, I was adding company D and was sure that there were going to be more. The solution which I decided on required rethinking the problem.

Move Logic to the view model.

The solution was that this logic should be in the ViewModel, this is the correct place for it, the ViewModel knows for this company where each property should be shown and where it should be hidden. The view should not care about all these different rules for each company. So in order to do this cleanly and that it would be readable I started by creating a new attribute:

public class HideThisAttribute : System.Attribute
{
    public IEnumerable<HideFor> Conditions { get; set; }    public HideThisAttribute()
    {
        Conditions = new List<HideFor> { HideFor.All };
    }    public HideThisAttribute(HideFor condition)
    {
        Conditions = new List<HideFor> { condition };
    }    public HideThisAttribute(HideFor[] conditions)
    {
        Conditions = conditions;
    }
}

This attribute takes a condition or a list of conditions in overloaded constructors. The conditions are just a simple enum named HideFor:

public enum HideFor
{
    All,
    Modal,
    NotDefault
}

Making an attribule means that for each ViewModel it is easy to add this attribute to the appropriate property as the following code example shows:

public class CompanyAAddressVM : IAddressVM
{
    [HideThis(new HideFor[] { HideFor.NotDefault, HideFor.Modal })]
    public override string PersonnelNumber { get; set; }
}

public class CompanyBAddressVM : IAddressVM
{
    [HideThis]
    public override string PersonnelNumber { get; set; }
}

public class CompanyDAddressVM : IAddressVM
{
    public override string PersonnelNumber { get; set; }
}

So now when looking at the ViewModel for the address of a certain company it is easy to see where a certain property should be hidden or shown. In the above case, the PersonnelNumber should be hidden for company A, if this is not the default address, or if this is a modal. For company B the PersonnelNumber should always be hidden whereas conversely for company D, the PersonnelNumber should always be shown.

Adding a Html Helper

To complete the removal of logic from the view I created a html helper which I named IsHiddenHelper:

public static class IsHiddenHelper
{
    public static bool IsHidden<TModel>(this HtmlHelper<TModel> html,
                                                             string propertyName, IAddressVM entity)
    {
        PropertyInfo prop = entity                                                
                                       .GetType()                                                
                                       .GetProperties()                                               
                                       .SingleOrDefault(o => o.Name == propertyName);
        if(prop != null)        
        {
            var attribute = prop.GetCustomAttribute(typeof(HideThisAttribute));
            if(attribute != null)
            {
                HideThisAttribute hideThis = (HideThisAttribute)attribute;
                
                if(hideThis.Conditions.Contains(HideFor.All))
                {
                    return true;
                }
                
                if(hideThis.Conditions.Contains(HideFor.Modal) && entity.IsModalAddress)
                {
                    return true;
                }                

                if(hideThis.Conditions.Contains(HideFor.NotDefault)
                                      && entity.IsDefaultAddress == false)
                {
                    return true;
                }
            }
        }        
        return false;
    }
}

This helper simply returns a true if the property is hidden, or a false otherwise, using this in the view means that the view becomes much cleaner and more readable. Here is the same view as at the top of this blog post, but using the new html helper. You can see that it is much clearer to understand:

@model Ch.Opten.Commerce.PresentationLayer.Interfaces.Models.Customer.IAddressVM
@if (!Html.IsHidden(Html.NameFor(model => model.PersonnelNumber).ToString(), Model))
{
    <div class="form-group">
    @Html.LabelRequired(Html.NameFor(model => model.PersonnelNumber).ToString(), null)
    @Html.TextBox(Html.NameFor(model => model.PersonnelNumber).ToString(),
                             Model.PersonnelNumber)
    @Html.ValidationMessage(Html.NameFor(model => model.PersonnelNumber))
    </div>
}
else
{
    @Html.Hidden(Html.NameFor(model => model.PersonnelNumber).ToString(),
                           Model.PersonnelNumber)
}

If the property is not hidden, then it will show the editor for this property, otherwise it will create a hidden input for this property. This new style as well as being cleaner is scalable, it can scale to 100 companies, and the view will not need to be changed.

Conclusions

What I am trying to show here is a pattern. If when developing you find that you are having a longer and longer list of cases that you have to check, maybe with a switch statement of multiple if statements. Then you may be in the position where your code has too much responsibility. You are handling logic which can be moved into separate classes. This is known as separation of concerns.

In this example the view did not need to be concerned with all the different companies. It only needed to be concerned with whether a property should be hidden or not. So the specific rules for each customer group, were moved into the customer group ViewModel. The result of applying separation of concerns to your code is that the code is much more extensible. In this case a new company can be added simply by adding a new ViewModel for that company, the view itself does not need to be changed.

Implementing separation of concerns in an old code base means that you probably have to think about the problem in a different way It may take a little more time but it pays of in the long run, making the code much more maintainable and extensible.


kommentieren


0 Kommentar(e):