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 <[email protected]> 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
>
