LeeCampbell / RxCookbook

Collection of recipes and snippets helping you create useful Rx code
281 stars 39 forks source link

Unnecessary Observable.Create() in OnPropertyChanges extension method #53

Closed activa closed 7 years ago

activa commented 7 years ago

In one of your recipes, you define an extension method to create an observer for property changes:

public static IObservable<TProperty> OnPropertyChanges<T, TProperty>(this T source, Expression<Func<T, TProperty>> property)
    where T : INotifyPropertyChanged
{
    return  Observable.Create<TProperty>(o=>
    {
        var propertyName = property.GetPropertyInfo().Name;
        var propertySelector = property.Compile();

        return Observable.FromEventPattern<PropertyChangedEventHandler, PropertyChangedEventArgs>(
                        handler => handler.Invoke,
                        h => source.PropertyChanged += h,
                        h => source.PropertyChanged -= h)
                    .Where(e => e.EventArgs.PropertyName == propertyName)
                    .Select(e => propertySelector(source))
                    .Subscribe(o);
    });
}

Is there a reason why you wrap this in a call to Observable.Create() ? If I understand correctly, the only difference this will make is that the two lines of code that get the property name and compile the property selector will be executed for every subscriber, which is not necessary and even hurts performance.

The following code does the same thing and prevents the calls to GetPropertyInfo() and .Compile() for every subscriber:

public static IObservable<TProperty> OnPropertyChanges<T, TProperty>(this T source, Expression<Func<T, TProperty>> property)
    where T : INotifyPropertyChanged
{
    var propertyName = property.GetPropertyInfo().Name;
    var propertySelector = property.Compile();

    return Observable.FromEventPattern<PropertyChangedEventHandler, PropertyChangedEventArgs>(
                    handler => handler.Invoke,
                    h => source.PropertyChanged += h,
                    h => source.PropertyChanged -= h)
                .Where(e => e.EventArgs.PropertyName == propertyName)
                .Select(e => propertySelector(source));
}

Am I missing something obvious?

LeeCampbell commented 7 years ago

My stance on this is that calling a method that returns an IObservable<T> should do nothing. It is the subscription to that returned value that should invoke any side effects or processing. If a subscription cost needs to be amortised across numerous subscriptions then the various multicast options in Rx should be applied. In this specific case, the argument is weak, however it then muddies the pattern and opens the door for other methods to "do work" without a subscription. I see this as a very common mistake and a source of race conditions and leaking resources (think opening a connection, starting a timer etc)