Creating list ViewModels in the correct way I'm using Caliburn Micro to create a WPF application. 

So what I want to do here is a typical Master/Detail situation. I'm displaying a list of Users and you can can add/edit a User and save the changes back to the database.

Everything works, but I'm just not sure if I'm creating the UserViewModels in the correct way. I lay awake at night yearning for a more elegant way.

Anything else that I'm doing wrong please let me know. It greatly helps me out and I appreciate every comment I get. Even if you must call me an idiot. Many thanks in advance!

PS: I left a few implementation details out to keep it brief. But let me know if anything else would help.

public class UserWorkspaceViewModel : Conductor&lt;UserViewModel&gt;.Collection.OneActive
{
    private IUnitOfWork _unitOfWork;
    private UserViewModelFactory _userViewModelFactory;

    public UserWorkspaceViewModel(IUnitOfWork unitOfWork, IUserViewModelFactory userViewModelFactory)
    {
        _unitOfWork = unitOfWork;
        _userViewModelFactory = userViewModelFactory;
        LoadUsers();
    }

    public void LoadUsers()
    {
        var users = _unitOfWork.Users.GetByDepartmentId(1);
        foreach (User user in users)
        {
            // Use the factory to create the ViewModel
            UserViewModel viewModel = _userViewModelFactory.CreateInstance(user);

            // Caliburn Micro specific: add user to the screen collection
            this.Items.Add(viewModel);
        }
    }
}


Here's the UserViewModelFactory implementation:

public class UserViewModelFactory
{
    private IUnitOfWork _unitOfWork;

    public UserViewModelFactory(IUnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork;
    }

    public UserViewModel CreateInstance(User user)
    {
        UserViewModel vm = new UserViewModel(_unitOfWork);

        // Use AutoMapper to map properties from user to VM
        Mapper.Map&lt;User, UserViewModel&gt;(user, vm);

        return vm;
    }
}


And finally here's the UserViewModel:

public class UserViewModel : Screen
{
    private IUnitOfWork _unitOfWork;

    #region Properties
    // Properties like FirstName, LastName, etc.
    #endregion

    public UserViewModel(IUnitOfWork unitOfWork)
    {
        _unitOfWork unitOfWork;
    }

    public void Save()
    {
        if (this.Id == 0)
        {
            User user = Mapper.Map&lt;UserViewModel, User&gt;(this);
            _unitOfWork.Users.Add(user);
            _unitOfWork.SaveChanges();
        }

        if (this.Id &gt; 0)
        {
            User user = _unitOfWork.Users.GetById(this.Id);
            Mapper.Map&lt;UserViewModel, User&gt;(this, user);
            _unitOfWork.SaveChanges();
        }
    }
}
UserWorkspaceViewModel  


private UserViewModelFactory _userViewModelFactory; instead of using the actual type use the interface like private IUserViewModelFactory _userViewModelFactory;  
magic number 1 (Users.GetByDepartmentId(1);) should be expressed by a meaningful constant.  
LoadUsers() method. Here IMHO you have used the var wrong. I don't think it is obvious what the right hand side of var users = _unitOfWork.Users.GetByDepartmentId(1); can be.  

List&lt;User&gt; users = _unitOfWork.Users.GetByDepartmentId(1);
foreach (var user in users)
{

}  


would be more obvious.  

UserViewModel 


the constructor won't compile, but I assume this is a copy &amp; paste problem.  

public UserViewModel(IUnitOfWork unitOfWork)
{
    _unitOfWork = unitOfWork;
}  

Save() method  

I guess it won't be possible for the Id property to become &lt;0 so a simple else instead of another if would be better.
If the chances are high that you will need to add more code, consider to extract the adding and updating to separate methods.  

General  

Comments should explain why something is done. What is done should be explained by the code itself by using meaningful names for classes,methods and parameters. So comments like // Use the factory to create the ViewMode should be removed.