Hi,
I agree with Dawid, modulo that I don’t have any preference about point 2 - I’m
ok even with not enforcing this.
One side note about point 4. There are use cases where passing obligatory
parameters in the build method itself might make sense:
I. - when those parameters can not be or can not be easily passed via the
constructor. Good example of that is “builder” pattern for the StreamOperators
(StreamOperatorFactory), where factory is constructed on the API level in the
client, then it’s being serialised and sent over the network and reconstructed
on the TaskManager, where StreamOperator is finally constructed. The issue is
that some of the obligatory parameters are only available on the TaskManager,
so they can not be passed on a DataStream level in the client.
II. - when builder might be used to create multiple instances of the object
with different values.
Piotrek
> On 26 Aug 2019, at 15:12, Jark Wu <imj...@gmail.com> wrote:
>
> Hi Gyula,
>
> Thanks for bringing this. I think it would be nice if we have a common
> approach to create builder pattern.
> Currently, we have a lot of builders but with different tastes.
>
>> 1. Creating the builder objects:
> I prefer option a) too. It would be easier for users to get the builder
> instance.
>
>> 2. Setting properties on the builder:
> I don't have a preference for it. But I think there is another option might
> be more concise, i.e. "something()" without `with` or `set` prefix.
> For example:
>
> CsvTableSource source = new CsvTableSource.builder()
> .path("/path/to/your/file.csv")
> .field("myfield", Types.STRING)
> .field("myfield2", Types.INT)
> .build();
>
> This pattern is heavily used in flink-table, e.g. `TableSchema`,
> `TypeInference`, `BuiltInFunctionDefinition`.
>
>> 3. Implementing the builder object:
> I prefer b) Mutable approach which is simpler from the implementation part.
>
>
> Besides that, I think maybe we can add some other aspects:
>
> 4. Constructor of the main class.
> a) private constructor
> b) public constructor
>
> 5. setXXX methods of the main class
> a) setXXX methods are not allowed
> b) setXXX methods are allowed.
>
> I prefer both option a). Because I think one of the reason to have the
> builder is that we don't want the constructor public.
> A public constructor makes it hard to maintain and evolve compatibly when
> adding new parameters, FlinkKafkaProducer is a good example.
> For set methods, I think in most cases, we want users to set the fields
> eagerly (through the builder) and `setXXX` methods on the main class
> is duplicate with the methods on the builder. We should avoid that.
>
>
> Regards,
> Jark
>
>
> On Mon, 26 Aug 2019 at 20:18, Gyula Fóra <gyula.f...@gmail.com> wrote:
>
>> Hi All!
>>
>> I would like to start a code-style related discussion regarding how we
>> implement the builder pattern in the Flink project.
>>
>> It would be the best to have a common approach, there are some aspects of
>> the pattern that come to my mind please feel free to add anything I missed:
>>
>> 1. Creating the builder objects:
>>
>> a) Always create using static method in "built" class:
>> Here we should have naming guidelines: .builder(..) or
>> .xyzBuilder(...)
>> b) Always use builder class constructor
>> c) Mix: Again we should have some guidelines when to use which
>>
>> I personally prefer option a) to always have a static method to create the
>> builder with static method names that end in builder.
>>
>> 2. Setting properties on the builder:
>>
>> a) withSomething(...)
>> b) setSomething(...)
>> c) other
>>
>> I don't really have a preference but either a or b for consistency.
>>
>>
>> 3. Implementing the builder object:
>>
>> a) Immutable -> Creates a new builder object after setting a property
>> b) Mutable -> Returns (this) after setting the property
>>
>> I personally prefer the mutable version as it keeps the builder
>> implementation much simpler and it seems to be a very common way of doing
>> it.
>>
>> What do you all think?
>>
>> Regards,
>> Gyula
>>
