Xamarincommunitytoolkit: [Enhancement] Fluent Builder Pattern For Bindable Properties

[aguevara716]

Summary

I'm personally not a fan of the BindableProperty.Create method with its many optional parameters - it is VERY difficult to figure out which parameters you need, which ones you have omitted, etc. I think using a fluent builder pattern would make it easier to create a new BindableProperty.

API Changes

~New class: BindablePropertyParameters:~ Edit: removed

~And~ BindablePropertyBuilder:

public class BindablePropertyBuilder
{
    private Type declaringType;
    private BindingMode defaultBindingMode;
    private object defaultValue;
    private string propertyName;
    private Type returnType;

    private BindableProperty.BindingPropertyChangedDelegate bindingPropertyChangedDelegate;
    private BindableProperty.BindingPropertyChangingDelegate bindingPropertyChangingDelegate;
    private BindableProperty.CoerceValueDelegate coerceValueDelegate;
    private BindableProperty.CreateDefaultValueDelegate createDefaultValueDelegate;
    private BindableProperty.ValidateValueDelegate validateValueDelegate;

    public BindablePropertyBuilder();
    public BindablePropertyBuilder SetReturnType(Type returnType);
    public BindablePropertyBuilder SetPropertyName(string propertyName);
    public BindablePropertyBuilder SetDefaultValue(object defaultValue);
    public BindablePropertyBuilder SetDefaultBindingMode(BindingMode bindingMode);
    public BindablePropertyBuilder SetDeclaringType(Type declaringType);

    public BindablePropertyBuilder SetValidateValueDelegate(BindableProperty.ValidateValueDelegate validateValueDelegate);
    public BindablePropertyBuilder SetPropertyChangedDelegate(BindableProperty.BindingPropertyChangedDelegate propertyChangedDelegate);
    public BindablePropertyBuilder SetPropertyChangingDelegate(BindableProperty.BindingPropertyChangingDelegate propertyChangingDelegate);
    public BindablePropertyBuilder SetCoerceValueDelegate(BindableProperty.CoerceValueDelegate coerceValueDelegate);
    public BindablePropertyBuilder SetCreateDefaultValueDelegate(BindableProperty.CreateDefaultValueDelegate createDefaultValueDelegate);

    public BindableProperty Build();
}

Intended Use Case

Simply put, instead of using the current Create method

public static readonly BindableProperty MyBindableProperty = BindableProperty.Create(nameof(MyProperty), typeof(string), typeof(MyControl), string.Empty, BindingMode.TwoWay, OnMyPropertyChanged)

developers can use a fluent builder syntax

public static readonly BindableProperty MyBindableProperty = new BindablePropertyBuilder()
    .SetPropertyName(nameof(MyProperty))
    .SetDeclaringType(typeof(MyControl))
    .SetReturnType(typeof(string))
    .SetDefaultValue(string.Empty)
    .SetDefaultBindingMode(BindingMode.TwoWay)
    .SetPropertyChangedDelegate(OnMyPropertyChanged)
    .Build();

Who Will Do The Work?

• [x] I am willing to take this on myself (I have already implemented this on a personal project)
• [ ] Just putting this out there for someone to pick up

[jfversluis]

Thanks for your great suggestion! The C# for Markup extensions should be coming from Forms to the toolkit. Do they do this already?

[aguevara716]

As far as I can tell, no, Xamarin doesn't have a fluent syntax for building an instance of BindableProperty - it is simply instantiated through the Create method and its huge list of parameters.

[roubachof]

This is a good idea. BindableProperty.Create has poor quality regarding readability.
Instead of the Create() method of the BindablePropertyBuilder, I would rather use Build(), since this is a builder :)

[aguevara716]

Agreed & updated, @roubachof!

[pictos]

Moving the discussion in PR to here. @aguevara716

@pictos understood - I had something similar in mind, but I didn't want to make too many assumptions.

My only concern about creating a singleton is that it would be "dirty" between usages (in other words, it would retain the BindablePropertyParameters from the previous use). Unless the final Build() method resets these parameters, but then the BindablePropertyParameters field would no longer be readonly.

Alternatively, I was thinking of the following approach:

public class BindablePropertyBuilder
{
    readonly BindalbePropertyParameters bpParameters;

    BindablePropertyBuilder() => bpParameters = new BindablePropertyParameters();

    public static BindablePropertyBuilder InitializeWithPropertyName(string propertyName)
    {
        var bpb = new BindablePropertyBuilder();
        bpb.bpParameters.PropertyName = propertyName;
        return bpb;
    }

    // TODO add the rest of the Set*() methods and the Build() method
}

Looks like that we have a new BindablePropertyBuilder per creation. I really want to avoid that, because creating objects in mobile is _evil_ we want to avoid allocations as much as we can. Since BindalbePropertyParameters is a private field I don't see any trouble removing the readonly and do something like:

// Implementation1

class Builder
{
     BindablePropertyParameters bpParameters = new BindablePropertyParameters ();

    public BindableProperty Build()
   {
       // Do the build things
      bpParameters = new BindablePropertyParameters ();
   }
}

This way we can one instance of the builder and clean-up the BindablePropertyParameters object.

[aguevara716]

@pictos

creating objects in mobile is _evil_

Can you expand on this? Not that I'm disagreeing with you, but I'd love to learn more.

[roubachof]

@pictos what would be the difference between creating an instance of BindablePropertyBuilder or an instance of BindablePropertyParameters? At the end of the day you are still creating a new object.

And, I would rather say that allocating a JNI Peer object is evil, but a pure C# object is totally fine. It will be well processed by the C# GC.

[pictos]

@aguevara716, of course, I can explain what it's in my mind. And don't need to be on the defensive, you can disagree or ask anything to me!

In general, for .NET when you create objects they are allocated in the heap, and when you don't need them anymore the GC runs and clean-up those objects. But GC collections are expensive, and we want to avoid then when we can.
Since we are working on a lib that a lot of devs will use, with different scenarios, I believe that we should care about perf. and don't allocate a lot of objects is a win.
Now, for Builders doesn't make sense, to me, to have one builder instance per object creation, BP MyBP = new BPBuilder(), to me makes more sense to the Builder be a static class or a singleton (just one object during the execution of the app).

@pictos what would be the difference between creating an instance of BindablePropertyBuilder or an instance of BindablePropertyParameters? At the end of the day, you are still creating a new object.

@roubachof, you are right about the new object creation, but now will be just one and not two (per call) and that is an improvement. If we can just set BindablePropertyParameters would be an improvement! But I need to take time on it to bring a spec. Also since BindablePropertyParameters is just a _model_ and looks like to be immutable we can try to change it to a struct and see what happens.

And, I would rather say that allocating a JNI Peer object is evil, but a pure C# object is totally fine. It will be well processed by the C# GC.

At the end of the day, allocate objects is a bad thing for perf. here we are looking just in one class and one BP. Let's expand this scenario... Imagine that we change all BPs of the media element control to use the builder in the way that is implemented today, so we have something like this per BP:

public static readonly BindableProperty SomePropProperty = new BindablePropertyBuilder().;

// this going on

This way we will have one object for the BP and for the BindablePropertyBuilder (two allocations per BP). Now imagine an app like instagram that you can have a lot of media elements in a list, this starts to be a problem.

Now this is just a theory, I not sure about the next assumption, we need to run a profiler on it

Also, this will be easily GarbageCollected? Because we may have a BindablePropertyBuilder with a strong reference to the BP and this can leads to a memory leak(?)

P.S.: I'm sorry for the late reply, I can answer new comments by tomorrow.

[roubachof]

you are right about the new object creation, but now will be just one and not two (per call) and that is an improvement. If we can just set BindablePropertyParameters would be an improvement!

@pictos Oh I didn't understand I thought it was either using a singleton with BindablePropertyParameters or the builder without this inner object :)
@aguevara716 looking at your implementation what is the purpose of the BindablePropertyParameters? Why just not put the fields in the builder?

Also, this will be easily GarbageCollected? Because we may have a BindablePropertyBuilder with a strong reference to the BP and this can leads to a memory leak(?)

I don't see how this could happen since there is literally 0 reference to the builder. Just a call to the Build() method.
Given the static context of the bindable properties, I'm not even sure it will reach the GC nursery. Worst case scenario it will stay shortly in gen0 before being collected right away. So I really don't think the allocation of this object will be an issue here.

[ahoefling]

@pictos

I believe that we should care about perf. and don't allocate a lot of objects is a win.

I think this is the most important thing to consider. I would be interested in seeing the DotNetBenchmarking results of standard BindableProperty vs the Fluent syntax. That will give us definitive results if we can accept the change and understanding any performance implications.

Echoing what @roubachof said, I don't understand why we need BidnablePropertyParameters. When I have used the Fluent API pattern in the past I always make my properties instance variables instead of creating a 2nd object.

@pictos
I am trying to fully understand your argument for a static object for the BindablePropertyBuilder.

• How do you see this being thread safe?
• Is this something we should worry about?

The first thing that comes to mind for me is creating a double locking thread safe singleton, but I am hesitant to add any locks to this code because that could create load time performance issues downstream. This is why I am recommended instantiating a new builder object each time.

[pictos]

@ahoefling

How do you see this being thread safe?

To be honest I didn't think about it. On my head (and maybe I'm being naive), BPs are for controls, and controls are created in UIThread (if you try to do some control action in BackgroundThread you'll see an exception). So no chance for concurrent access. Am I right?

Is this something we should worry about?

As I said Builders should be one instance, if you have to create a new builder per object it lost the purpose, I think. But if you have concerns about it we can create a static method to CreateBuilder and let the final user handle that, if he wants to use one instance or create a new one.
I revised here and we have a StringBuilder that creates a new object per builder... But I don't know if it's the same case... But if we don't need to worry about thread safe we can move with a static builder, right?

Also BindableProperty.Create is a static method

[AndreiMisiukevich]

IMO, I don't think that the allocation of small builder objects will cause a significant performance penalty.

[ahoefling]

@pictos

controls are created in UIThread (if you try to do some control action in BackgroundThread you'll see an exception). So no chance for concurrent access. Am I right?

I don't think this is an assumption we can make anymore with dual screen devices. Considering a dual screen device across different platforms will they have 1 and only 1 UI Thread, I would argue maybe and that is a no leaning maybe. When I was doing research on this topic prior to the release of the Surface Duo I discovered at the time Android supported 1 UI Thread and marshaled between the different activities being displayed on the screen. Conceptually it was similar to javascript mult-threading. That is no longer the case and Android does support multiple UI Threads for dual screen devices. If we have an app that is built for a dual screen device and has 2 windows being displayed at once, do we get 2 singletons or 1? Based on what I have seen so far this scenario would create a concurrency problem.

In addition to all of that, XCT will be used on desktop apps as Xamarin.Forms supports UWP, MacOS, and even Linux using GTK#. I don't think it is right to make the assumption that threading concerns can be ignored. I do understand the point of view and if you asked me this question 2 years ago, I would have had a different anwer.

[pictos]

@ahoefling I updated my last comment with references, you can take into account the fact that BindableProperty.Create is a static method. So the builder it's just a sugar syntax to access this method, so we the builder can be static as well. Is that make sense or am I missing something else?

[roubachof]

For me the simpler the better. Since the 2 solutions have each 1 heap allocation, just having one class instead of two is simpler. As I said earlier, there will be no gc issues here.

[pictos]

@aguevara716 can you update your spec? Also, this issue will be just for BindableProperty.Create or will have others?

[aguevara716]

@pictos @roubachof

Just to be 100% clear, we're going with the following:

• Get rid of the BindablePropertyParameters class, move all of those properties into BindablePropertyBuilder as private fields
• Make the BindablePropertyBuilder a singleton, and the .Create() method will instantiate a new BindablePropertyBuilder with the default parameter values

@pictos - I think I can add in the other 3 BindableProperty.*Create* methods. I'm debating how it should be implemented, though. Unfortunately, 2 of the Create methods have its own set of default values (I'm not sure which method, but I know it's the default BindingMode parameter that differs), and the BindablePropertyKey is not the base class/derived class for BindableProperty. This is why I'm hesitant. But my approach would be to either have 4 total BindablePropertyBuilder classes, or 4 Builder.Build methods.

[roubachof]

Get rid of the BindablePropertyParameters class, move all of those properties into BindablePropertyBuilder as private fields

Yup, I think everyone agree on that.

Make the BindablePropertyBuilder a singleton, and the .Create() method will instantiate a new BindablePropertyBuilder with the default parameter values

well if you ask me, the first version of the api was fine:

public static readonly BindableProperty EventNameProperty = new BindablePropertyBuilder()
            .SetPropertyName(nameof(EventName))
            .SetReturnType(typeof(string))
            .SetDeclaringType(typeof(EventToCommandBehavior))
            .SetPropertyChangedDelegate(OnEventNamePropertyChanged)
            .Build();

Simple and clear.

[roubachof]

The only reason I would use a builder factory here (a static method returning the concrete builder instance), would be if we need an abstract builder (different concrete instances implementing an interface) . But since we only need one type of instance, new operator is clearer and simpler.

[aguevara716]

@roubachof thanks for clearing that up!

@roubachof @pictos I updated the spec

[aguevara716]

@pictos et al. what do you think of the following implementation for supporting the Create, CreateAttached, CreateReadOnly, and CreateAttachedReadOnly methods?

https://gist.github.com/aguevara716/2caae5da9ab5b0d50b93514c29449e2d

The usage would be the same as laid out in the spec, only you would specify which type of BindableProperty you would create by using a different builder class.

Example:

public BindableProperty MyRegularBindableProperty 
    = new BindablePropertyBuilder()./*set parameters here*/.Build();

public BindablePropertyKey MyReadOnlyBindableProperty 
    = new BindablePropertyKeyBuilder()./*set parameters here*/.Build();

public BindableProperty MyAttachedBindableProperty 
    = new AttachedBindablePropertyBuilder()./*set parameters here*/.Build();

public BindablePropertyKey MyAttachedReadOnlyBindableProperty 
    = new AttachedBindablePropertyKeyBuilder()./*set parameters here*/.Build();

[pictos]

@aguevara716 I'll take a look at this weekend. @StephaneDelcroix if you can share your thoughts here will be awesome, I don't know too much about the Binding world. Thanks in advance