reactor-netty
reactor-netty copied to clipboard
Flawed naming conventions
Take a look at this API
client
.headers(builder -> {
builder.add(COOKIE, new DefaultCookie(SESSION_ID, "hello"));
})
.post()
.uri("/uri")
.send(ByteBufFlux.fromString(just("hi")))
.response((httpClientResponse, byteBufFlux) -> {
return just(sessionIdAndApiKey);
});
When you look at the method post()
, your intuition tells that the POST
call is being made. But as per API, you are building a request that uses http POST
method when the actual subscription
happens & the request building ends with send
AFAIK
This is a flawed api design as it breaks intuition. The same is true for all other http methods aswell. This flaw first appeared in WebClient
API of Webflux & is now carried to reactor-netty
aswell
Any action methods get()
, post()
, e.t.c would be considered by most developers as ACTION methods, not builder methods
A suggestion I have is to name these methods use<Op>
(or) <op>Builder()
(or) for<Op>
, i.e client.useGet()
(or) client.getBuilder()
(or) client.forGet()
Spring Data R2DBC also has exactly the same issue
Also, send
method too feels like an ACTION, but the actual call happens when we subscribe like any other reactive stream (please correct me if I'm wrong). So, even send
method also needs to be renamed as we are in the reactive world, not imperative world
Instead of send
, we need to name it such that it informs the user that we will be sending body whenever subscription happens, may be requestBody
instead of send
& formBody
instead of sendForm
?
We followed the same API guidelines than reactor and transitively functional API especially rx. Would you think that Flux.map() directly maps something ? or Flux.merge(a, b) ? The consistency you see between projects is definitively on purpose as we are trying to avoid different API model mindsets where possible to make the whole thing easier to learn and port between projects.
The important thing is what is it that api will return that might help clear the confusion, e.g. a Flux returns a Flux, send will return a ResponseSpec. In untyped languages that might be more challenging, but Java at least has the type system on its side.
Don't get me wrong, I can see where how you can get it wrong without context, that's why its important to have clear guidelines and consistent apis. I don't think we'll change anytime soon reactor-core or webclient for instance so it would be preferred better for reactor-netty to align.
However we still have an open window to finalize the API before 1.0 is stamped and we can continue discussing. Appreciate your input tho and I like requestBody/formBody (maybe requestForm for discoverability). We'll try to collect more feedbacks internally and externally and align with R2DBC also young project with opportunities to improve :)
@smaldini Thanks for your response
We need some guidelines on naming methods in reactive world so that regular java users do not feel they are caught off guard. We need rules for what kind of names to avoid (blacklist) in reactive world
I have few scattered thoughts on this. One such scattered thought is
If the method looks like an imminent action & also it affects how the user perceives about the method's behaviour based on his past imperative model (I agree we are not in imperative model, but most of us are coming to reactive model from this old notions & the method names in imperative model says what it does, here .get()
does not actually get anything now, but an alias for http GET), we need to be a bit cautious about its name
For e.g, take your examples of flux.map(..)
& merge(..)
. In these examples, does the methods map
& merge
affect how the user's perception about when map
& merge
happens? Even if does affect the when, will this have any impact in user's mental model about how this affects the behaviour of the method chain? Does it impact his previous understanding he is bringing in from his imperative model? My subjective perception is, not as much as .send
, .get()
x.get()
, .post()
, .send()
, .execute()
, .fetch()
, .run()
feels imminent & have a sense of immediacy to them, unlike map
& merge
. I am sure these are my subjective feelings & others might not feel the same
Plus, once we have fibres in Java 12+, we would want to retain the immediacy of the above terms to mean immediate execution in a fibre that does not block the thread (this might or might not happen, but kotlin async
model does something similar to this)
Overall there seems to be a need for us to frame some rule of thumb about how not to name a method in reactive world as imperative world is not going away anytime soon
regular java users do not feel
I think this is the underlying assumption in the whole discussion. Regular translates to imperative programming.
If the method looks like an imminent action … affect how the user's perception about when (map & merge) happens?
This again depends on the context. There is a point as .block()
or .subscribe()
aren't substantially different from a name like .send()
. The only thing that distinguishes here is knowledge how Project Reactor building blocks are supposed to work.
I also do not think there is really anything that we can do about it without breaking the entire API. Maybe we can take away that execute()
and run()
(verbs strongly related to execution/invocation) should be considered more carefully when building new reactive API.
.block()
does what it implies from method name. It blocks current thread when the method gets executed
.subscribe()
does it what implies based on method name. It subscribes to the stream the method gets executed
but .send()
does not actually send. I think this is where the grey area is
The only thing that distinguishes here is knowledge how Project Reactor building blocks are supposed to work.
I too agree with this statement partially, only partially, as there few operators in reactor (not the ones mentioned above) that are not as straight forward as block
& subscribe
. For e.g take replay
. If I apply the same question (action principle) I started with, we need to ask "when does this replay happen? when replay method is invoked (or) only after the subscription"
So you are right that domain context matters, but can the methods be named such that, method names such as send
be replaced with some name that indicates what it does, at the same time does not indicate when it does it
For e.g like my earlier suggestion requestBody
instead of send
(note that its only one example, may be others can comeup with better suggestions)
A rough guideline I've in my mind is this
Method names should say
what
they do, avoid implyingwhen
they do it
Method names like execute
, fetch
(r2dbc), get
, post
, send
(the current library), e.t.c breaks this guideline as they imply what
partially, but imply when
strongly
Avoid method names that imply when
along with what
, unless there are no better names
Please note that these are my rough thoughts & I might be going in the wrong direction completely. I'm only trying to see if we can make life of the rest of 95-99% java (imperative) programmers life simple when they transition
Also, method names that imply when
along with what
that are on streams are somewhat understandable as there is a ground rule which says subscribe
is the entry point for all streams
But, Builders are not streams. So having a method name imply when
within builder method is even more confusing
I applied this when
vs what
guideline to adba method names rowOperation
& countOperation
. As you can see they only indicate what
they do, and does not imply when
they do it. They could have named the method count
instead of countOperation
, but in that case, the method name would have implied when
along with what
I am not sure if this is the actual reason/there is someother reason why adba named countOperation
instead of just count
. Irrespective of the intent, the adba method names clearly does not indicate immediacy