kobylynskyi / graphql-java-codegen

Make your GraphQL Java application schema-driven.
https://kobylynskyi.github.io/graphql-java-codegen/
MIT License
268 stars 115 forks source link

chained setters #1389

Open FyiurAmron opened 11 months ago

FyiurAmron commented 11 months ago

Is your feature request related to a problem? Please describe. chained setters, akin to lombok.accessors.chain ; quote from Lombok's docs:

chain – A boolean. If true, generated setters return this instead of void. Default: false

Describe the solution you'd like A boolean option making it possible to use chained setters.

Describe alternatives you've considered Not using chained setters :)

jxnu-liguobin commented 11 months ago

Does it look like builder pattern?

FyiurAmron commented 11 months ago

@jxnu-liguobin on the surface level yes, because "chained setters" are an example of fluent syntax and builders commonly also use fluent syntax, but the use case is completely different.

Builder pattern should cover only immutable data objects, and thus is completely useless when the data is mutable by design, i.e. when the setters are expected to be called after object creation, not during it. Also, some people use builders with fluent syntax with no set prefix (I'd say this is the most common example), some use the prefix, and some (albeit that's very rare) just don't use fluent syntax at all in builders (it's not mandatory for a proper GoF builder). Also, Lombok defined "fluent" as the 1st case, i.e. set/get with no prefix (which can cause trouble due to conflicting method overloads).

tl;dr chained setters improve the syntax of changing state for existing objects (via fluent syntax), builders improve the syntax of creating new objects (usually also via fluent syntax).

Further reading: e.g. https://medium.com/miro-engineering/fluent-setter-breaking-the-convention-33ce3433126e

jxnu-liguobin commented 11 months ago

It looks like a scala copy method, which is really useful for modifying. The difference is that scala's copy creates a new object, while this issue expects to modify the original object? So it only Java needs to be considered.

FyiurAmron commented 11 months ago

@jxnu-liguobin

It looks like a scala copy method, which is really useful for modifying. The difference is that scala's copy creates a new object,

I'd say that in Scala you don't even have to, you can just

{
  import boxVar._
  setLength(7)
  setHeight(3)
}

etc. - I'm not a Scala expert, but supposedly that works :D

while this issue expects to modify the original object?

Well, basically yes - some languages have the with (or other similar) construct either built-in somehow or provided via lib to deal with immutables, see https://docs.oracle.com/javase/8/docs/api/java/time/LocalDate.html#i60 e.g. , but this is for the regular mutables in Java ecosystem.

So it only Java needs to be considered.

honestly I'd say that you're right it mostly affects Java; both Kotlin and Scala have language built-ins that make chained setters almost completely obsolete, e.g. Kotlin has https://kotlinlang.org/docs/scope-functions.html#apply (see above for Scala) etc. - however, Java doesn't, which makes the code unnecessarily lengthy and hard-to-read, esp. if the to-be-changed variable has a long name etc.

rwo-trackunit commented 8 months ago

Chained setters as a concept

Chained setters fills a gap where you want to have the shorthand that builders give.

builder.firstPropery(1)
.secondPropery(List.of())
.thirdPropery(null)

but you don't want to treat the object as immutable.

Without chained setters, once the object has been build, the only reasonable way to set multiple fields in Java is

object.setFirstPropery(1)
object.setSecondPropery(List.of())
object.setThirdPropery(null)

which is not that pretty.

With chained setters, we can omit the repeated object reference:

object.setFirstPropery(1)
    .setSecondPropery(List.of())
    .setThirdPropery(null)

This is not quite fluent, as we keep the set prefix. Fluent would strip the set and get prefix, to match the builder interface.

Personal opinion.

Given that these generated models are data transfer objects (DTO), I don't see why you would avoid immutability, but we already accepted this by having setters in the first place. If the set method would return a copy, each step would use memory, so that option seems unwise. There is no performance difference between setting each property on the object reference, and using chained setters. This is purely visual code enhancement. On the other hand this is not a breaking change, as setter returns void currently.

How to

There seem to be two ways to support this:

Support chained accessors:

Return this on java setters. None breaking. Should be as simple as updating the ftl files fx. here. This solution could be a good first issue.

Support fluent style getter and setter.

Support fluent style getter and setter. The change here would be more involved, as it is breaking. So 1) Add setting to the project 2) Only prefix get or set if this setting is off (default) 3) Add return of this to setters.

Fluent has an inherit danger of name clashes, as properties are not prefixed. Fx. having a property with the same name as the surrounding object is no longer possible. This is also not what OP asked for.

jxnu-liguobin commented 8 months ago

I rarely use Java, but maybe we can support setField()/field() at the same time