Is it an anti-pattern to let ViewModel fill itself from a domain object? Let's say you have a Employees table mapped to a Employee class. Now you want to code a Edit page for general information. You need a ViewModel with all the properties you need to build the page, so you create EditEmployeeViewModel. Then you make your action EditEmployee and your view EditEmployee.

So far so good, but as you add more and more properties, all the code to fill your ViewModel ends up in your controller and as your edit page grows, your controller becomes more and more messy. Specially when your ViewModel starts depending on multiple models and/or lists. You also get the same problem when you get the data back; you have to update all the properties of entities involved in the model.

So my question is: Is it an anti-pattern or a bad practice to let the ViewModel map itself from the domain layer object(s) passed in the constructor or a method? If so, what would be a cleaner approach?

public class EmployeesController : Controller
{
    public ActionResult EditEmployee(int id)
    {
        var employee = repo.GetEmployee(id);
        return View(new EditEmployeeViewModel
        {
            Id = employee.Id,
            FirstName = employee.FirstName,
            LastName = employee.LastName
        });
    }
}




public class Employee
{
    public int Id { get; set; }
    public int FirstName { get; set; }
    public int LastName { get; set; }
    public int EmployeeNumber { get; set; }
}




public class EditEmployeeViewModel
{
    public int Id { get; set; }

    [StringLength(20, MinimumLength = 2)]
    public string FirstName { get; set; }

    [StringLength(20, MinimumLength = 2)]
    public string LastName { get; set; }
}




Example with the mapping in the ViewModel

public class EditEmployeeViewModel
{
    public int Id { get; set; }

    [StringLength(20, MinimumLength = 2)]
    public string FirstName { get; set; }

    [StringLength(20, MinimumLength = 2)]
    public string LastName { get; set; }

    public EditEmployeeViewModel(Employee employee)
    {
        Id = employee.Id;
        FirstName = employee.FirstName;
        LastName = employee.LastName;
    }

    public Employee UpdateModel(Employee employee)
    {
        employee.Id = Id;
        employee.FirstName = FirstName;
        employee.LastName = LastName;
        return employee;
    }
}
It's not an anti pattern. At some point, one layer must know about the layer underneath. You also don't want to rewrite view-model mapping code in multiple controllers or actions, nor do you want to introduce another layer to do the mapping for you. Instead, I opt for constructor injection.

The part of your code I do think is an anti pattern, or incorrect architecture, would be your view model having access to the data access layer.

Your last example using constructor injection will provide the least coupling and most code reuse:

public class EditEmployeeViewModel
{
    public EditEmployeeViewModel(Employee employee)
    {
        Id = employee.Id;
        // ...
    }
}


I would not include the reverse mapping in your view model, where you update an Employee domain model from the EditEmployeeViewModel object. Instead, I would opt for a command or service object to do this work, which would have access to the data access layer:

public class CreateEmployeeCommand
{
    public CreateEmployeeCommand(IEmployeeRepository repository, EditEmployeeViewModel model)
    {
        this.repository = repository;
        this.model = model;
    }

    private EditEmployeeViewModel model;
    private IEmployeeRepository repository;

    public void Execute()
    {
        if (repository.FindBySSN(model.SocialSecurityNumber) != null)
        {
            // cannot add duplicate employee
        }

        Employee employee = new Employee()
        {
            FirstName = model.FirstName,
            LastName = model.LastName,
            SocialSecurityNumber = model.SocialSecurityNumber,
            // ...
        };

        repository.Create(employee);
    }
}


Your controller becomes simplified. The key here is your data access layer should be created and destroyed at the controller level, not the command level. The controller should be controlling the transaction:

public class EmployeesController : Controller
{
    public ActionResult Create(EditEmployeeViewModel model)
    {
        if (!ModelState.IsValid)
            return View(model);

        using (IEmployeeRepository repo = new EmployeeRepository())
        {
            var command = new CreateEmployeeCommand(repo, model);

            command.Execute();

            repo.Commit();
        }

        return RedirectToAction("Index");
    }
}