Flawed naming convention

[thekalinga]

Take a look at this

databaseClient.execute()
  .sql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  .bind("$1", 42055)
  .bind("$2", "Description")
  .bindNull("$3", Integer.class)
  .fetch()

The method name execute() sounds & feels like we are executing something, not building a query for execution. For comparison, just try to understand what is fetch & what is execute in general. Both are ACTIONs, but the API expects us to treat them differently for no good reason. For a user who knows nothing about Spring Data R2DBC, execute API is not at all intuitive

I saw the same flawed naming convention being followed in WebClient api of webflux when it comes to building http requests, i.e inorder to build a GET request, they expect us to write webClient.get(), but this sounds & feels like we are making GET call when we are writing it (as per intuition), but we with WebFlux api we are not making the call with get, rather we are informing the API to return a *Builder/*Spec that will eventually make a GET request. Its a flawed API design as it breaks intuition

I have couple of suggestions about it

Either name methods that build something with op<Builder>() (or) for<Op>, i.e for building execute statements use databaseClient.executeBuilder() (or) databaseClient.forExecute()

The first one is more direct & does not leave any space for any confusion

[thekalinga]

Same applies to select & insert aswell

[thekalinga]

Also, looks like the actual call to db does not get initiated when fetch method is executed rather when the actual subscription occurs like any other reactive stream. So we need to rename this too so as not to give the user wrong intuition

[NorbertSandor]

Same applies to select & insert aswell

I'm not sure of this... Maybe it would be enough to start the chain with a more specifically named method like you suggested.

[thekalinga]

I'm implying using selectBuilder/forSelect for select just like for execute. I assume thats what you meant aswell

[mp911de]

Thanks a lot for raising the issue.

DatabaseClient and WebClient use a naming scheme for functional-reactive operators that let you compose a functional pipeline. Execution (the actual doing) happens much later upon subscription.

this sounds & feels like we are making GET call when we are making it (as per intuition),

The intuition originates in a different programming model. In imperative programming method names should translate to actions, to do something. We're in functional-reactive programming here. In functional-reactive programming, we expect methods to create/provide a functional stage that is appended to form a functional sequence.

DatabaseClient was created to align with WebClient and to provide the same look and feel for database access.

Please note that both client implementations are not builders – builders keep a shared mutable state to build a final object. DatabaseClient and WebClient produce immutable intermediate objects that are safe to be shared.

We're happy about receiving naming suggestions that fit into the functional-reactive programming context. We won't go back to an imperative variant.

[thekalinga]

@mp911de Thanks for your response

However, the core objection remains unanswered. As it is neither about calling builder (or) not, nor whether the intermediate objects are mutable (or) immutable, rather its about method names breaking intuition

Also, intuition has nothing to do with whether we are writing declarative (or) imperative programming

APIs might call them *Specs/*Builders, but end of the day they are storing the information about how to build the reactive chain when the actual subscription happens. My only objection is, doing so should not break intuition

As we all know, code is written once, read multiple times, if we follow some naming convention that does not break this intuition, its better for users. We need to break intuition with extreme care. I dont know if there is a good reason for breaking this intuition, other than using this convention (action name like select) can shorten method names

Also, since we use IDE, it does not matter if the method name is few characters long

Do you agree on whether looking at the code with action names that dont actually do any action breaks intuition?

[thekalinga]

I even go so far as to say, since declarative programming is relatively difficult to follow when compared to imperative, we should avoid making it confusing by avoiding (atleast sidestepping) action method names unless we are certain it performs a specific action

For e.g, take Mono.block(). Tho its part of reactive API, From method name, we can be certain that it blocks the current thread [I know its slightly an outlier, but it explains the crux of the problem] & is an action

[mp911de]

I think there's a point to this discussion and I have a feeling that it's mostly around the execute method. We're aiming towards a readable DSL. Looking at the example above gives us exactly that:

databaseClient.execute()
  .sql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  .bind("$1", 42055)
  .bind("$2", "Description")
  .bindNull("$3", Integer.class)
  .fetch()

Translates into "execute SQL INSERT …, bind parameters and then fetch the result as first/all" elements.

The same goes for WebClient:

webClient.get()
  .uri("http://host/{name}", name)
  .retrieve()
  .bodyToMono(Details.class);

Get some data from http://… and retrieve it as Details.

Note: get is a method name and can also be used as verb. Same goes for post and delete, only WebClient.method(…) breaks the scheme.

Playing a bit with naming and nesting, we could end up with things like:

databaseClient.newExecute()
  .sql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  …
 .fetch()

databaseClient.query()
  .sql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  …
 .fetch()

databaseClient.forSql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  …
 .fetch()

Adding prefixes here and there can improve naming in the first place. It adds a bulkhead that can help guide new users. Once used to this API style, users will find that bulkhead rather hinders readability.
With DatabaseClient and WebClient we're creating a new fluent approach to access data and HTTP resources. Each API/programming model requires a learning curve, and we do not imply a know-nothing approach that would enable using the API. Rather, learning from the ref docs and Javadoc is the way to explore the API.

I've two further thoughts here:

1. What is the actual problem are we trying to solve by changing naming?
2. What is the alternative?

Further references:

• MongoDB Fluent API

[gregturn]

I was getting the impression is that this is more about the deferred execution of reactive streams not being clearly visible. It implies that execute should be something like executeUponSubscription.

Since ALL of RS programming is deferred execution, we'd have to apply this to every part of this fluent API, and that would clearly get tedious at a certain point. Especially after new users have gotten the hang of RS.

I feel such nomenclature adds no more readability than if RestTemplate.exchange were put inside a Java 8 Consumer via lambda call. RestTemplate.exchange, upon execution, does its thing. WHEN it does it is dependent upon context outside the exchange method itself. The same could be said for WebClient of DatabaseClient. WHEN it happens isn't pertinent to the name.

[thekalinga]

@mp911de @gregturn

Sure,

I agree everything is context dependent, but looking at this databaseClient.execute()....fetch() feels so unnatural (ofcourse, its subjective) when it never executes & does not fetch unless someone subscribes in the future

I'm using RxJava for almost 2 years now & Reactor for atleast a year. AFAIK, there are no APIs from those two libraries that sacrifice readability (compose & transform are exceptions, which will be renamed in the future) unlike web-flux, reactor-netty & the current library

Compare these three & see which of them accurately says (more accurately does not say) what it does & when it does

databaseClient.execute()
  .sql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  .bind("$1", 42055)
  .bind("$2", "Description")
  .bindNull("$3", Integer.class)
  .fetch()

vs

databaseClient.forSql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  .bind("$1", 42055)
  .bind("$2", "Description")
  .bindNull("$3", Integer.class)
  .done()

vs

databaseClient.forExecute()
  .sql("INSERT INTO legoset (id, name, manual) VALUES($1, $2, $3)")
  .bind("$1", 42055)
  .bind("$2", "Description")
  .bindNull("$3", Integer.class)
  .done()

Please note that this is just a quick suggestion I could think of that does not confuse the users, but may be if we think about it a bit, we might comeup with a compromise that does not confuse at the same time try and not sacrifice the attributes you are trying to preserve in the new API

Also note that I'm only trying to use this as a starting point to make sure that 90-95% of developers who comes to reactive programming from regular Java/Spring programming should need feel method does not do what it says in the name

I agree with the direction to make the API fluent (I too am in favor of it), but not at the cost of intuition

[thekalinga]

ADBA APIs countOperation [Used for both insert & update statement] (or) rowOperation [used for select statement] also looks reasonably OK when its comes to intuition, as it they dont specify/imply when the method gets executed, but execute() & fetch() imply an operation that will happen now, not sometime in the future

While thinking about it, I was looking at the method bind. Even that method looks a bit off, but not as off as execute & fetch as when binding happens is usually immaterial from the point of view of the developer (as we dont have lazy evaluation of bound arguments as of now in the BindingSpec API)

Since this is a new area for most of us, there are no recipes to follow when it comes to naming things in this reactive world. May be we can attempt to comeup with some initial direction when it comes to this

[thekalinga]

Adding prefixes here and there can improve naming in the first place. It adds a bulkhead that can help guide new users. Once used to this API style, users will find that bulkhead rather hinders readability.

Take Flux.toStream for example. they explicitly added to prefix for readability reasons. They could have verywell named it as Flux.stream aswell, but dint do it for readability reasons AFAIK

Edit: I'm not sure if I'm completely accurate about above analysis, but it feels to be natural to name methods so that they accurately represent what they do