A reusable Previous/Next Contact Collection ViewModel class

Question

I often use a ViewModel class that contains a collection of Contacts and allows me to iterated through the collection with Previous and Next buttons.

I find that I've got duplicate versions of this class all over the place and I'd like to make my code reusable.

How would I go about making the following code reusable (i've been going round in circles for a while now):

public class PartiesViewModel : BaseViewModel
{
    const int MaxParties = 20;

    public ObservableCollection<Party> Parties { get; set; }

    public PartiesViewModel(ObservableCollection<Party> parties)
    {
        Parties = parties;
        Party = Parties.FirstOrCreateIfEmpty();

        Party.PropertyChanged += UpdatePartyShortNamesList;
        Parties.CollectionChanged += Parties_CollectionChanged;

        ClearCommand = new RelayCommand(ClearPressed, CanClear);
        NextCommand = new RelayCommand(NextPressed, CanPressNext);
        PrevCommand = new RelayCommand(PrevPressed, CanPressPrev);
    }

    void UpdatePartyShortNamesList(object sender, System.ComponentModel.PropertyChangedEventArgs e)
    {
        if (e.PropertyName == "ShortName")
            NotifyPropertyChanged("PartyShortNames");
    }
    void Parties_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        NotifyPropertyChanged("PartyNames");
        NotifyPropertyChanged("PartyShortNames");
        NotifyPropertyChanged("Count");
    }

    Party _Party;
    public Party Party
    {
        get { return _Party; }
        set
        {
            _Party = value;
            NotifyPropertyChanged("Party");
            NotifyPropertyChanged("Count");
        }
    }

    public string Count
    {
        get
        {
            var list = Parties.ToList();
            list.RemoveAll(x => x.IsEmpty());
            int current = list.FindIndex(x => x == Party) + 1;
            int count = list.Count();
            if (current == 0)
                return string.Format("{0} of {0}", count + 1);
            else
                return string.Format("{0} of {1}", current, count);
        }
    }

    public RelayCommand PrevCommand { get; set; }
    public bool CanPressPrev() { return IndexOf(Party) > 0; }
    public void PrevPressed()
    {
        try
        {
            int i = IndexOf(Party);
            Party = Parties[i - 1];
        }
        catch (Exception ex) { Message.ShowError(ex); }
    }

    public RelayCommand NextCommand { get; set; }
    public bool CanPressNext() { return !Party.IsNullOrEmpty() && IndexOf(Party) != MaxParties; }
    public void NextPressed()
    {
        try
        {
            if (IndexOf(Party) + 1 == Parties.Count)
            {
                Party = new Party();
                Parties.Add(Party);
            }
            else
            {
                int i = IndexOf(Party);
                Party = Parties[i + 1];
            }
        }
        catch (Exception ex) { Message.ShowError(ex); }
    }

    public RelayCommand ClearCommand { get; set; }
    public bool CanClear()
    {
        return !Party.IsNullOrEmpty();
    }
    public void ClearPressed()
    {
        try
        {
            int i = IndexOf(Party);
            if (Party == Parties.Last())
                Party.Clear();
            else
            {
                Parties.RemoveAll(x => x == Party);
                Party = Parties[i];
            }
        }
        catch (Exception ex) { Message.ShowError(ex); }
    }

    int IndexOf(Party party) { return Parties.ToList().FindIndex(x => x == party); }

    public static List<string> PartyTypes { get { return new List<string>() { "Individual", "Company", "Partnership" }; } }

    public bool IsCompany { get; set; }
}

Answer 1

A couple of comments:

• It looks like the class only manages parties. This is nice SRP.
• Use StyleCop, it makes code much easier to read and review.

• Validate inputs, makes it much faster to find and fix bugs:

  public PartiesViewModel(ObservableCollection<Party> parties)
  {
      if (parties == null)
      {
          throw new ArgumentNullException(nameof(parties));
      }
      ...
  

• Party = Parties.FirstOrCreateIfEmpty(); without knowing what this extension method does I don't love it as it requires me to navigate to another place in the code to see what it does. I guess it does something like this:

  Party = Parties.FirstOrDefault() ?? new Party(...);

  If this is the case I think you should drop the extension method.

• Party.PropertyChanged += UpdatePartyShortNamesList;

  I can't see that this subscription is updated anywhere. I would expect the subscription to be removed from the old party and added to the new party in the setter for Party. As it is now it is probably a bug and a memory leak.

• public ObservableCollection<Party> Parties { get; set; } a public setter here suggests that the collection can be replaced (without notifying the view about it). I assume you want it to be a readonly property. This also applies to the command properties.

  In C#6 you express this as:

  public ObservableCollection<Party> Parties { get; }

  Prior to C#6 this was slightly more verbose:

  private readonly ObservableCollection<Party> _parties;
  
  public PartiesViewModel(ObservableCollection<Party> parties)
  {
      _parties = parties;
  }
  
  public ObservableCollection<Party> Parties
  {
      get { return _parties; }
  }
  

  Being explicit about what is readonly is a very good habit as it makes code easier to reason about and lets the compiler catch more bugs.

• I have a feeling that binding SelectedIndex can simplify things a lot.

• The command methods CanPressPrev etc. shall probably be private as the commands are normally the exposed means to interact with the viewmodel.

• About reuse, this class looks like a nice candidate for composing with. Inject it in other viewmodels where the contact list is needed.

This got long, ending here.