3
\$\begingroup\$

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<UserViewModel>.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<User, UserViewModel>(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<UserViewModel, User>(this);
            _unitOfWork.Users.Add(user);
            _unitOfWork.SaveChanges();
        }

        if (this.Id > 0)
        {
            User user = _unitOfWork.Users.GetById(this.Id);
            Mapper.Map<UserViewModel, User>(this, user);
            _unitOfWork.SaveChanges();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Just for future reference, we prefer that you leave those details in. However, I think this question looks fine without them. \$\endgroup\$ – RubberDuck Dec 3 '14 at 0:02
1
\$\begingroup\$
  • 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<User> 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 & paste problem.

      public UserViewModel(IUnitOfWork unitOfWork)
      {
          _unitOfWork = unitOfWork;
      }  
      
    • Save() method

      I guess it won't be possible for the Id property to become <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.

\$\endgroup\$
1
\$\begingroup\$

I personally like keeping my post-constructor invariants readonly and ensuring they are valid with a guard clause as such:

public class UserViewModel : Screen
{
    private readonly IUnitOfWork _unitOfWork;

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

    public UserViewModel(IUnitOfWork unitOfWork)
    {
        if (unitOfWork == null)
        {
            throw new ArgumentNullException("unitOfWork");
        }

        _unitOfWork = unitOfWork;
    }

    // the rest
}
\$\endgroup\$

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.