2

My simplified program structure looks like this:

public class Manager
{
    public Item MyItem { get; set; }

    public void Recalculate(){ ... } 
}

public class Item
{
    public string SomeProperty { get; set; }
}

public class ManagerViewModel
{
    public Manager Model { get; set; }

    public ItemViewModel MyItem { get; set; }
}


public class ItemViewModel
{
    public Item Model { get; set; }

    public string SomeProperty
    {
        get => Model.SomeProperty;
        set 
        { 
            Model.SomeProperty = value;
            RaisePropertyChanged("SomeProperty");
        }
    }
}

When SomeProperty gets changed in ItemViewModel, I want Recalculate() to get triggered inside Manager.

Do I:

A) Have a PropertyChangedListener inside ManagerViewModel which listens for Property changes inside it's MyItem, and then tells it's Model to Recalculate()

B) Allow ItemViewModel to have access to Manager, so it can manually tell Manager to run Recalculate()

..

(B) seems kind of anti-pattern-ish... shouldn't each ViewModel only really be concerned with it's own Model? (A) has it's own issues -- I need to use this sort of 'Recalculation' structure a lot, and it seems having these PropertyChangedListeners all over the place is kind of messy. I realise there's a few different ways of going about this, but I'm just wondering what the 'best practice' is in this case.

amputek
  • 59
  • 1
  • 6
  • 2
    A is much preferred. B is an abomination — the opposite of loose coupling. A cleaner version might be to give ItemViewModel another event that it raises only when it changes the value of a property that’ll trigger a recalculation. But then again, only the parent VM really knows which properties it’s aggregating, so maybe just handle PropertyChanged and leave ItemViewModel out of it. – 15ee8f99-57ff-4f92-890c-b56153 Oct 27 '17 at 23:01
  • Agreed it’s messy. But sometimes it is. – 15ee8f99-57ff-4f92-890c-b56153 Oct 27 '17 at 23:04
  • I see! I do quite like the custom event approach. I think the issue with having RaisePropertyChanged everywhere is that it now does two things: It tells the UI to update, and it triggers Recalculate(). Most of the time it seems fine that both of these things happen.. but there might be cases where you'd want one to happen and not the other. – amputek Oct 28 '17 at 00:07
  • 1
    *"It tells the UI to update, and it triggers Recalculate()"* -- not at all! It says "To whom it may concern: This value changed." That's only one thing. Multiple handlers out there in the darkness may be listening in for their own uniquely nefarious purposes, but that's OK. "I may be recalculated, so here's my recalculate event" is a second thing; "I may change, so here's my change event" is just one very pure and basic and generalized thing. Why should ItemViewModel know or care about aggregate functions? – 15ee8f99-57ff-4f92-890c-b56153 Oct 28 '17 at 00:10
  • Ok, this makes sense. I'm starting to get my head around it all - thanks! I wonder what I'd do in a case where 'ItemViewModel' has a command which, for example, resets all of it's Properties (and it's Model's properties). Realistically, I'd want to be able to reset all of the properties, and *then* have Recalculate() run, as it would be inefficient to run it after every single property change. Would an event be appropriate in this case? – amputek Oct 29 '17 at 18:54
  • IIRC (you should confirm with MSDN) (I'm on my way out the door, sorry!), raising PropertyChanged with null for a property name signifies that all properties should be refreshed. If that is the case, I'd set the private backing fields for all of them and then call OnPropertyChanged(null) (or maybe write an overload of OnPropertyChanged for that case, if you're using `CallerMemberName`). If that's not a standard thing with INPC, then a special event would make sense there. – 15ee8f99-57ff-4f92-890c-b56153 Oct 29 '17 at 22:00

2 Answers2

2

As confirmed by Ed Plunkett, 'option A' is the best approach, as it separates the concerns of the ViewModels and Models.

ItemViewModel is only concerned with it's own Model, and it just notifies whoever's listening that it's properties have been changed.

ManagerViewModel listens for changes inside ItemViewModel, and then executes Recalculate() inside it's own model.

//Models

public class Manager
{
    public Item MyItem { get; set; }

    public void Recalculate(){ ... } 
}

public class Item
{
    public string SomeProperty { get; set; }
}

//ViewModels

public class ManagerViewModel
{
    public Manager Model { get; set; }

    public ItemViewModel MyItem { get; set; }

    public ManagerViewModel()
    {
        //Listen for property changes inside MyItem
        MyItem.PropertyChanged += ItemPropertyChanged;
    }

    //Whenever a Property gets updated inside MyItem, run Recalculate() inside the Manager Model 
    private void ItemPropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        Model.Recalculate();
    }
}


public class ItemViewModel
{
    public Item Model { get; set; }

    public string SomeProperty
    {
        get => Model.SomeProperty;
        set 
        { 
            Model.SomeProperty = value;
            RaisePropertyChanged("SomeProperty");
        }
    }
}
amputek
  • 59
  • 1
  • 6
1

There's nothing wrong with making your Model(s) observable by adding INotifyPropertyChanged to them also. Then you can listen to the models you're bound to. In many projects I prefer to have a static data set layer, which publishes the list of models from a store and those models are all observable. This means they can be bound to and since the store is the same source any ViewModels etc can bind to them and become updated system wide. Looks like you're on the right track so don't second guess yourself. Making things observable, downright to the model, isn't bad or considered bad practice. I personally prefer it.

Michael Puckett II
  • 5,915
  • 5
  • 21
  • 41
  • I actually had something similar to that. My issue was that there were cases when setting a Model's Properties where I didn't want the UI to update (straight away, at least). For example, I have an xml file importer which populates the Model's properties - I wouldn't want to Notify the UI (and in my case, 'Recalculate()') after every single Property change - just once, after the loading has finished. Likewise there are cases (non-UI cases) where I'd want to set groups of Properties and *then* have the UI update. – amputek Oct 28 '17 at 02:32
  • Well, there's more to it than that in my opinion. I personally never expose Model's to the UI although many patterns and examples disagree. I always wrap Models in a new ViewModel no matter how small for that very reason. Because you never know and exposing Models to the UI directly is just something I always avoid. – Michael Puckett II Oct 28 '17 at 02:35
  • Ah, maybe I misunderstood your answer. Are you saying Models should (could) observe each other? In my example above would 'Manager' listen for Property Changes in it's 'Item' in order to run Recalculate()? – amputek Oct 28 '17 at 02:46
  • hmmm, not necessarily, maybe I miss understood the question lol. Model's themselves can be observable and bound to yes but I would always keep a dependency direction in mind when binding to them. EF breaks this rule some, and that's sad, but making Models bind to each other probably isn't the best idea. I would let ViewModels handle that logic for you and then the View could respond to the ViewModels. – Michael Puckett II Oct 28 '17 at 02:58
  • So in my answer I would have you bind to the models in the viewmodels and then perform the calculations there when the models change and update the properties on the ViewModels where the Views would then be listening and reacting. – Michael Puckett II Oct 28 '17 at 02:59
  • Here's an answer I gave with my personal suggestion on MVVM. It breaks up MVC and MVVM so have a read if you're interested. Not sure if the link will go directly to my answer or not but if you look I have an answer there. Hopefully it helps. https://stackoverflow.com/questions/667781/what-is-the-difference-between-mvc-and-mvvm/41843901#41843901 – Michael Puckett II Oct 28 '17 at 03:01