reactor / reactor-netty

TCP/HTTP/UDP/QUIC client/server with Reactor over Netty
https://projectreactor.io
Apache License 2.0
2.57k stars 640 forks source link

Flawed naming conventions #561

Open thekalinga opened 5 years ago

thekalinga commented 5 years ago

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()

thekalinga commented 5 years ago

Spring Data R2DBC also has exactly the same issue

thekalinga commented 5 years ago

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?

smaldini commented 5 years ago

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 :)

thekalinga commented 5 years ago

@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

mp911de commented 5 years ago

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.

thekalinga commented 5 years ago

.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)

thekalinga commented 5 years ago

A rough guideline I've in my mind is this

Method names should say what they do, avoid implying when they do it

thekalinga commented 5 years ago

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

thekalinga commented 5 years ago

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