Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I've been reading Clean Code and various online articles about SOLID lately, and the more I read about it, the more I feel like I don't know anything.

Let's say I'm building a web application using ASP.NET MVC 3. Let's say I have a UsersController with a Create action like this:

public class UsersController : Controller
{
    public ActionResult Create(CreateUserViewModel viewModel)
    {

    }
}

In that action method I want to save a user to the database if the data that was entered is valid.

Now, according to the Single Responsibility Principle an object should have a single responsibility, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility. Since validation and saving to the database are two separate responsibilities, I guess I should create to separate class to handle them like this:

public class UsersController : Controller
{
    private ICreateUserValidator validator;
    private IUserService service;

    public UsersController(ICreateUserValidator validator, IUserService service)
    {
        this.validator = validator;
        this.service= service;
    }

    public ActionResult Create(CreateUserViewModel viewModel)
    {
        ValidationResult result = validator.IsValid(viewModel);

        if (result.IsValid)
        {
            service.CreateUser(viewModel);
            return RedirectToAction("Index");
        }
        else
        {
            foreach (var errorMessage in result.ErrorMessages)
            {
                ModelState.AddModelError(String.Empty, errorMessage);
            }
            return View(viewModel);
        }
    }
}

That makes some sense to me, but I'm not at all sure that this is the right way to handle things like this. It is for example entirely possible to pass an invalid instance of CreateUserViewModel to the IUserService class. I know I could use the built in DataAnnotations, but what when they aren't enough? Image that my ICreateUserValidator checks the database to see if there already is another user with the same name...

Another option is to let the IUserService take care of the validation like this:

public class UserService : IUserService
{
    private ICreateUserValidator validator;

    public UserService(ICreateUserValidator validator)
    {
        this.validator = validator;
    }

    public ValidationResult CreateUser(CreateUserViewModel viewModel)
    {
        var result = validator.IsValid(viewModel);

        if (result.IsValid)
        {
            // Save the user
        }

        return result;
    }
}

But I feel I'm violating the Single Responsibility Principle here.

How should I deal with something like this?

share|improve this question
    
Shouldn't the user class handle the validation? SRP or not, I don't see why the user instance should not know when it is valid or not and rely on something else to determine that for it. What other responsibilities does the class have? Plus when user changes the validation will probably change, so outsourcing that to a different class will only create a tightly coupled class. –  sebastiangeiger Nov 12 '11 at 16:22
add comment

2 Answers

up vote 3 down vote accepted

I really don't think you are violating Single-Responsibility-Principle in your second example.

  • The UserService class has only one reason to change: if there is a need to change the way you save a user.

  • The ICreateUserValidator class has only one reason to change: if there is a need to change the way you validate a user.

I must admit that your first implementation is more intuitive. However, the validation should be done by the entity that creates the user. The creator itself shouldn't be responsible for validation; it should rather delegate the responsibility to a validator class (as in your second implementation). So, I don't think the second design lacks SRP.

share|improve this answer
1  
Single-Responsibility-Pattern? Principle, perhaps? –  Yannis Rizos Nov 12 '11 at 7:38
    
Yes, of course :) Corrected it! –  Guven Nov 12 '11 at 16:12
add comment

It seems to me that the first example is "closer" to true SRP; it's the responsibility of the Controller in your case to know how to wire things up and how to pass along the ViewModel.

Wouldn't it make more sense to have the whole IsValid/ValidationMessages be part of the ViewModel itself? I haven't dabbled with MVVM, but it seems like the old Model-View-Presenter pattern, where it was okay for the Presenter to handle things like that because it was directly related to presentation. If your ViewModel can check itself for validity, there's no chance to pass an invalid one to the UserService's Create method.

share|improve this answer
    
I always thought ViewModels should be simple DTO's without too much logic in them. I'm not sure if I should put something like checking the database in a ViewModel... –  Kristof Claes Oct 7 '11 at 18:55
    
I guess it would depend on what your validation entails; if the ViewModel just exposes the IsValid boolean and ValidationMessages array, it could still call into a Validator class and not have to worry about how the validation is being implemented. The Controller could check that first e.g. if (userViewModel.IsValid) { userService.Create(userViewModel); } Basically the ViewModel should know if it is valid, but not how it knows it's valid. –  Wayne M Oct 7 '11 at 20:48
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.