Self populating view models

So I have a view model (asp.net mcv 3 app) that when instantiated with a particular parameter, populates it's properties via a couple of calls to a web service in it's constructor.

public class QuizPreviewViewModel
{
    private IQuizServiceWrapper _service;
    public string QuizName { get; set; }
    public List QuizQuestions { get; set; }
    public List AnswerChoices { get; set; }
    public int QuizId { get; set; }

    public QuizPreviewViewModel()
    {
    }

    public QuizPreviewViewModel(int quizId )
    {
        _service = new QuizServiceWrapper();
        var quiz = _service.GetQuizHeader(quizId);
        var questions = _service.GetQuizQuestionsByQuiz(quizId).OrderBy(x => x.Order).ToList();
        QuizId = quizId;
        QuizName = quiz.QuizName;
        QuizQuestions = questions;
    }
}

I know I could just as easily perform the population in the controller, or possibly extract the code to populate the properties into a separate method or helper class.

I have read in various books/articles that both view models and controllers should be kept as clean as possible. So I'm wondering what you all think of populating the view model in it's constructor. Is this a decent practice, or a horrible idea? Where is the best place to populate the view model?

Personally I try to keep the view models as close to DTO's as possible. That way you can have any number of objects populating the data from any number of sources. And, as you mentioned, it helps keep them lean and free of domain logic.

If there was only one particular method of doing the transformation I might look at existing mapping solutions i.e. AutoMapper. Alternatively as you suggested I might create a separate builder class that was responsible for doing the building.

The main problem I have with doing it your way (apart from those mentioned by Larry already) is that it tightly couples the ViewModel with your model. Re-using the viewModel in different contexts becomes harder.

Also, if you were to to TDD, I might find it a bit strange writing a test like:

public void TestMyViewModel()
{
   try
   {
      // Act
      var viewModel = QuizPreviewViewModel(1);

      // nothing to arrange???

      // Assert
      Assert.AreEqual(1, viewModel.QuizId);
   }
   catch(Exception ex)
   {
      Assert.Fail();
   }   
}

I think I would prefer something like:

public class QuizPreviewViewModelBuilder
{
    private readonly IQuizServiceWrapper _service;

    public QuizPreviewViewModel(IQuizServiceWrapper service)
    {
        _service = service;        
    }

    public QuizPreviewViewModel GetViewModel(int quizId)
    {
        var viewModel = new QuizPreviewViewModel(); 
        var quiz = _service.GetQuizHeader(quizId);

        viewModel.QuizId = quizId;
        viewModel.QuizName = quiz.QuizName;
        viewModel.QuizQuestions = quiz.questions;

        return viewModel;
    }
}

then your test class would look like:

public void TestMyViewModelBuilder()
{
      // Act
      var mockService = // new mock service however this works
      var builder = QuizPreviewViewModelBuilder(mockService);

      // arrange
      var viewModel = builder.GetViewModel(1);

      // Assert
      Assert.AreEqual(1, viewModel.QuizId);   
}

Even this has a potential downside I guess in the fact that you have to have a bit of knowledge as to how to build up the service in order for the GetViewModel to work, so I'm not completely sold on that idea.

Although this really just moves the logic of the building from the controller to another class I think it better allows for re-useability of the ViewModel while also providing a DRY approach to doing the actual building.

Just a couple of different thoughts for you to mull over.

StackExchange Code Review Q#15188, answer score: 6