1
\$\begingroup\$

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; }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. I'd recommend you ask a follow-up question if you want a review of the code you edited into your question ;) \$\endgroup\$ – Vogel612 Mar 2 '16 at 10:05
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Great tips, cheers. Any thoughts on how I can make this class generic or based on an Interface? Today i'm using the Party object, but tomorrow it will be a Contact (or something else). I ran into trouble because I create a New object in both the constructor and the NextCommand. \$\endgroup\$ – Matt Fitzmaurice Feb 29 '16 at 10:53
  • \$\begingroup\$ For the new I see a couple solutions: 1) pass in a Func<T> crateor 2) Constrain with where T : new meaning T must have a default ctor. 3) Let it be null and use FallBackValuein the bindings. \$\endgroup\$ – Johan Larsson Feb 29 '16 at 11:54

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.