2

I've seen this pattern used in a considerable amount of MVC applications. Let's assume we have a User model with the methods:

  • hasSessionExpired
  • displaySessionExpired

The first method (hasSessionExpired) looks up in the database if the user's session has expired and returns true or false and the second calls the first method and returns an HTML string that basically says "Expired" with a red font or "Active" with a green font depending on the output of hasSessionExpired.

In the controller there is a call to the User object to fetch all users which are returned as an array of User objects, this array is sent to the view.

Then in the view, the array is iterated and the method displaySessionExpired is called for every object in the array.

There is something that smells fundamentally wrong with this approach, since the model has presentation logic in the displaySessionExpired method and the outcome of that logic is based on the outcome of a database query made at that moment. This of course makes the model classes have thousands of lines plagued with mixes of presentation and business logic that in the long run makes the application more difficult to maintain.

How should this be refactored so there is a clear separation of concerns?

1

The 'displaySessionExpired' on your model simply does not belong there. It is the responsibility of your model to handle logic, but HOW it is displayed (what text and what color) is the responsibility of the view. It is up to the view to call hasSessionExpired and take the result to be displayed.

1

I think there's a missing concept here, which is UserSession. An user can have multiple UserSession (but only one active session at a time)

So if I do the refactoring, it would be something like this (in .NET):

public class UserSession
{
    private DateTime _expireTime;
    //Other properties

    public bool IsActive()
    {
        return _expireTime > DateTime.Now;
    }
}

public class User
{
    private List<UserSession> _sessions;

    public bool IsLoggedIn()
    {
        return _sessions.Any(s => s.IsActive());
    }
}

Those two are in domain layer

MVC controllers will get list of users from database, with UserSession loaded in User. If you are worrying about performance impact of loading all UserSession, you can just load active UserSession.

In MVC view, to display who's logging in, who's not:

@foreach (var user in users)
{
    if(user.IsLoggedIn())
    {
        <span style='background-color: green' >@user.Name</span>
    }
    else
    {
        <span style='background-color: red' >@user.Name</span>
    }
}

Or even better if you have a view model that contains all data needed to be displayed in the view:

public class UserStatusViewModel
{
    public string Name { get; set; }
    public bool IsLoggedIn { get; set; }
}

The mapping from User to UserStatusViewModel is done in controller/application service. The view now depends on view model instead of model

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.