3
\$\begingroup\$

Assume a viewmodel for hosting a collection of item viewmodels. There can be some hundreds of items in the collection. The items are part of the business model and have an id and a name. The name for a specific item can be changed somewhere in the application and the viewmodel in the collection needs to be notified to refresh the property:

public interface IItem
{
    Guid Id { get; }
    string Name { get; }
}

public class ItemModel : ViewModelBase
{
    private IItem _item;

    public ItemModel(IItem item)
    {
        _item = item;
    }

    public void Refresh()
    {
        RaisePropertyChanged(nameof(Name));
    }

    public Guid Id { get { return _item.Id; } }

    public string Name { get { return _item.Name; } }

    private bool _isSelected;

    public bool IsSelected
    {
        get { return _isSelected; }
        set { Set(ref _isSelected, value); }
    }
}

public class ListModel : ViewModelBase
{
    public ListModel(IEnumerable<ItemModel> items)
    {
        Items = new ObservableCollection<ItemModel>(items);
    }

    public ObservableCollection<ItemModel> Items { get; }
}

I have tried the following solutions, but i am not sure what the best one is:

  1. register every item at creation and let the messenger always notify them all

    public class ItemModel : ViewModelBase
    {           
        public ItemModel(IItem item)
        {
            _item = item;
    
            MessengerInstance.Register<NotificationMessage>(this, (msg) => {
                if (msg.Notification == "REFRESH" && IsSelected)
                {
                    Refresh();
                }
            });
        }
    
        [...]
    
  2. register / unregister messenger on selection, so there will be only one viewmodel be registered

    public class ItemModel : ViewModelBase
    {
        [...]
    
        public bool IsSelected
        {
            get { return _isSelected; }
            set
            {
                if (Set(ref _isSelected, value))
                {
                    if (value)
                    {
                        MessengerInstance.Register<NotificationMessage>(this, (msg) => {
                            if (msg.Notification == "REFRESH")
                            {
                                Refresh();
                            }
                        });
                    }
                    else
                    {
                        MessengerInstance.Unregister<NotificationMessage>(this);
                    }
                };
            }
        }
    }
    
  3. register the hostig viewmodel and iterate over the items, so there wil be only one registration too

        public ListModel(IEnumerable<ItemModel> items)
        {
            [...]
    
            MessengerInstance.Register<NotificationMessage<Guid>>(this, (msg) => {
                if (msg.Notification == "REFRESH")
                {
                    var item = Items.Single<ItemModel>(x => x.IsSelected);
                    item.Refresh();
                }
            });
        }
    

which solution would you prefer? register and unregister on every selection or having hundereds of viewmodels registered to the messenger or itereate the items yourself? maybe there is also a better solution?

\$\endgroup\$
  • \$\begingroup\$ Do you have hundreds of items in the collection or do you have hundreds of viewmodels? \$\endgroup\$ – t3chb0t Sep 18 '17 at 10:01
  • \$\begingroup\$ A collection with hundreds of viewmodels (one for each business model) that is bound to the ItemsSource-property of a ListBox that is part of a master-detail-view. maybe i should have mentioned that... \$\endgroup\$ – Qwertz123 Sep 18 '17 at 11:49
0
\$\begingroup\$

It should be up to that ViewModel how it handles messages and if it wants to ignore them or act on them. So that rules out option 3. That just leaves option 1 and 2. Option 2 will work but as a developer I wouldn't normally think that setting a property of IsSelected to have side effects and we all know side effects are bad. I just think it adds a bit more in the maintenance that doesn't need. I typically just do option 1 and I typically have one method in a class that handles all the registrations for the messages and what they should do. Just makes maintenance easier if you have a message issue you know where they are all getting registered at.

And since this is CodeReview and all code is subject to review I would recommend you get rid of "magic" strings and make a class that holds them all. Something like

public static class Messages
{
    public const string Refresh = "REFRESH";
}
\$\endgroup\$
  • \$\begingroup\$ i think you are right... i tried to reduce the number of messenger registrations, but i think its much easier to let the messenger itereate some more messeage handlers than the overhead of handle the register/unregister calls on selectionchanged-events and so on... \$\endgroup\$ – Qwertz123 Sep 25 '17 at 8:29

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.