Can this reactive code be improved?

Question

I have a view model that I'm migrating to RxUI. It exposes a collection of brokers along with a bool indicating whether there is a market (ie. at least one broker). In addition, it has a property for the currently selected broker (the user can choose a broker via a drop-down):

private IReactiveCollection<SourceViewModel> brokers;
public IReactiveCollection<SourceViewModel> Brokers
{
 get { return this.brokers; }
 private set { this.RaiseAndSetIfChanged(x => x.Brokers, ref this.brokers, value); }
}
private ObservableAsPropertyHelper<bool> isMarket;
public bool IsMarket
{
 get { return this.isMarket.Value; }
}
private SourceViewModel selectedBroker;
public SourceViewModel SelectedBroker
{
 get { return this.selectedBroker; }
 set { this.RaiseAndSetIfChanged(x => x.SelectedBroker, ref this.selectedBroker, value); }
}

I have these pieces hooked up and working, but I am not convinced I've done this in the most reactive-friendly fashion:

this.WhenAny(x => x.Volatilities, x => x.Value)
 .StartWith((ItemObservableCollection<VolatilityViewModel>)null)
 .Subscribe(x =>
 {
 if (x == null)
 {
 // no volatilities, so definitely no market
 this.Brokers = null;
 this.SelectedBroker = null;
 this.isMarket = Observable.Return(false).ToProperty(this, y => y.IsMarket);
 }
 else
 {
 // we have volatilities, so potentially we have a market
 // TODO: is there a better means of doing this?
 this.Brokers = x.CreateDerivedCollection(y => y.Source);
 // TODO: is there a cleaner way to do this?
 this.Brokers.Changed
 .Select(y => true)
 .StartWith(true)
 .Subscribe(y =>
 {
 var volatility = x.FirstOrDefault();
 this.SelectedBroker = volatility == null ? null : volatility.Source;
 });
 // TODO: is there a cleaner way to do this?
 this.isMarket = this.Brokers.CollectionCountChanged.Select(y => y > 0).StartWith(x.Count > 0).ToProperty(this, y => y.IsMarket);
 }
 });

My Volatilities property is set when the user interacts with a specific set of vols. Everything hinges off of it. It's currently an ItemObservableCollection, but I may change it later to be a reactive collection. I don't believe that's relevant here, but I could be wrong.

The parts I'm concerned about have TODOs in the code, but to summarize:

• when my Volatilities property changes, so too do the available brokers. I currently create a new derived collection each time Volatilities changes. I couldn't come up with any way to avoid this or implement it more cleanly.
• when I recreate my brokers collection, I immediately set up a Changed observer so that any changes result in the first broker being selected. The reasoning is that the first broker will (once I do sorting) be the preferred broker, so any changes in the market should instigate the selection of the preferred broker. I couldn't think of a way to make this logic cleaner and simpler.
• in addition, I also recreate my isMarket member, which is true whenever there is at least one broker in the brokers collection. Again, I'm not sure that this is the best approach to achieving this.

Can anyone weigh in with their thoughts on this?

• Eren Ersönmez: Thank you for this question, Kent. I think you just introduced me to Rx :)

Answer 1

I currently create a new derived collection each time Volatilities changes

This was a good idea, but in this case I'd just recreate the brokers list (since it's not a straight volatilities.Select(x => ...), it's a SelectMany. One thing that's important though, is whether you mutate (i.e. add and remove) items from Volatilities, or you only replace the list (i.e. via an API call, set this.Volatilities to a new collection).

Pick one or the other, don't mix them or else your code will be complicated. Let's assume you're using ReactiveCollection and you're adding/removing to Volatilities.

Volatilities.CollectionCountChanged.StartWith(Volatilities.Count)
 .Select(_ => Volatilities.SelectMany(x => x.BrokerList))
 .ToProperty(this, x => x.Brokers);

Then, we can watch when someone changes Brokers - we'll do this in the constructor (along with everything else). Generally, if you're not calling RxUI methods in the constructor, you're Doing Something Wrong. You're wiring up on startup what will happen when various parts of your ViewModel change, those wires typically are constant:

this.WhenAny(x => x.Brokers, x => x.Value)
 .Subscribe(_ => SelectedBroker = Brokers.FirstOrDefault());

Now let's set up the IsMarket:

this.WhenAny(x => x.Brokers, x => x.Value)
 .Select(x => x.Count > 0)
 .ToProperty(this, x => x.IsMarket);

What We Can LearnTM

The key point here though, is that IsMarket is directly related to Brokers and nothing else - we should describe that relationship without mentioning Volatilities, then separately describe the relationship between Volatilities and Brokers. We don't want to mix the two.

• Kent Boogaart: Thanks Paul - this makes a lot of sense. The situation is that Volatilities is set once for the life-span of the dialog interaction, during which time it may also be modified. I am currently re-using the one view model instance for multiple (synchronous) user interactions. Perhaps I'll be better off creating a new instance each time, passing in Volatilities to the ctor. I'll look at refactoring this and incorporating your above recommendations. Cheers.
• Kent Boogaart: Just got to looking at this (was pulled onto something else). I notice your code assumes Volatilities is ReactiveCollection<T> and not IReactiveCollection<T>, since the latter does not have a Count property. For obvious reasons I'd prefer to expose the interface, but seem to be in a bit of a bind. Same with Add/Remove. I can always use Count() extension method or add properties & methods to the type that contains the collection, but wondered at your thoughts on this...
• AlSki: You could use IReactiveCollection<T>.Any() instead of .Count()>0
• DanH: To echo Paul's comments, the rx chains should ideally be setup in the viewmodels constructor. You are then functionally defining all the behaviour one time at startup. If you are having to recreate these all the time then its going to get ugly fast. I don't user RX ui at the moment although am considering it as have rewritten rather a lot of it myself, and my preference would be to not blow away the collections each time, but instead reset them to new values so your behaviours don't need recreating. Not sure how you do that for brokers col in rxui, but must be do-able.