Hi Gyula, A few comments from my side.
Ad. 1 Personally I also prefer a static method in the "built" class. Not sure if I would be that strict about the "Builder" suffix, though. It is usually quite easy to realize the method returns a builder rather than the object itself. In my opinion the suffix might be redundant at times. Ad. 2 Here I also don't have a strict preference. I like the b) approach the least though. Whenever I see setXXX I expect this is an old style setter without return type. For that reason for a generic name I always prefer withXXX. I see though lots of benefits for the option c), as this might be the most descriptive option. Some examples that I like the usage of option c) are: * org.apache.flink.api.common.state.StateTtlConfig.Builder#useProcessingTime * org.apache.flink.api.common.state.StateTtlConfig.Builder#cleanupInBackground * org.apache.flink.api.common.state.StateTtlConfig.Builder#cleanupInRocksdbCompactFilter() To sum up on this topic I would not enforce a strict policy on this topic. But I am ok with one, if the community prefers it. Ad. 3 I agree that mutable Builders are way more easier to implement and usually it does not harm to have them mutable as long as they are not passed around. Some other topics that I think are worth considering: 4. Setting the default values: a) The static method/constructor should accept all the required parameters (that don't have a reasonable defaults). So that newBuilder(...).build() construct a valid object. b) Validation in the build() method. newBuilder().build() might throw an Exception if some values were not set. Personally I think the option a) should be strongly preferred. However I believe there are cases were b) could be acceptable. 5. Building the end object: a) Always with build() method b) Allow building object with arbitrary methods I think the option a) is the most common approach. I think though option b) should also be allowed if there is a good reason for that. What I can imagine is when we need some additional information from the build method (e.g. generic type) or if the method modifies the internal state of the Builder in a way that it is unsafe to continue setting values on the Builder. An overall comment. I think it is good to share opinions on this topic, but I am afraid there is too many sides to the issue to standardize it very strictly. I might be wrong though. Really looking forward to the outcome of this discussion. Best, Dawid On 26/08/2019 14:18, Gyula Fóra 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 >
signature.asc
Description: OpenPGP digital signature
