Hi all,
I'd like to differentiate between API level builder usage and "internal"
builder usage (for example, test harness).
For API level builder, in general everything goes, as long as it aligns
with user expectations. API level usages are also much more discussed, such
that I'd expect them to be consistent within one API. This freedom is
especially required when considering APIs for non-java languages.
Now for "internal" usages (which may or may not align with Java Datastream
etc. usage). I'd like to get a style that is well supported by the
primarily used IDEs. I don't want to write a new builder from scratch and
by the looks of it, we will get many more builders.
Furthermore, I'd like to emphasize that the primary use case for using
builders for me is to mitigate the lack of named arguments in Java, which
is especially painful for immutable types. In an ideal world, I'd like to
have all classes immutable and use builders to create new instances if we
have
a) too many parameters to be passed (which should become many more once we
commit even more to DI),
b) we have meaningful default values, such that we can omit a significant
amount parameters by using a buidler, or
c) we have a good amount of optional (=nullable) parameters.
Obviously, we deviate from that whenever performance considerations demand
it.
With that my votes for the questions:
1. static method only, hide builder ctor
2. Intellij and avro use setX() for property X, so I'd go with that. Lombok
just uses X(), so I wouldn't mind it.
3. Mutable approach. Immutable doesn't make much sense to me. Then I can
directly go with a Wither pattern on the immutable class without builder.
4. Private ctor. If it has a builder, it should be used. Exception:
migration support for some intermediate versions (if we added a builder to
a class, keep the ctor deprecated public for 1,2 versions).
5. no setX in general, we want to have an immutable class. Exceptions where
due (should be mostly for performance reasons).
Best,
Arvid
On Mon, Aug 26, 2019 at 4:40 PM Piotr Nowojski <pi...@ververica.com> wrote:
> 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
> >>
>
>
--
Arvid Heise | Senior Software Engineer
<https://www.ververica.com/>
Follow us @VervericaData
--
Join Flink Forward <https://flink-forward.org/> - The Apache Flink
Conference
Stream Processing | Event Driven | Real Time
--
Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany
--
Ververica GmbH
Registered at Amtsgericht Charlottenburg: HRB 158244 B
Managing Directors: Dr. Kostas Tzoumas, Dr. Stephan Ewen
