search

RocketChat / Rocket.Chat.Java.SDK

[DEPRECATED, NOT MAINTAINED] Java/Android SDK for Rocket.Chat
MIT License
29 stars 23 forks source link

Using Java 8 CompletableFuture instead of callback/listener #29

Closed daniel-sc closed 6 years ago

daniel-sc commented 7 years ago

Hi there,

really like that you provide a Java SDK!

Whilst using it, I realized that working with java.util.concurrent.CompletableFuture is really more convenient then handling all the listeners/callbacks. It allows for a much more readable coding style and gives more control as to which result belongs to which trigger.

This PR should introduce this to all relevant methods of RocketChatAPI. As the tests didn't run in the original version, I didn't bother to adapt them for my setup .. maybe you can check if the tests are still in the green?

Of course I'm happy to hear feedback or answer questions!

Best, Daniel

CLAassistant commented 7 years ago

CLA assistant check
All committers have signed the CLA.

luciofm commented 7 years ago

Hi @daniel-sc

This SDK is targeted for use on our Android App (and of course other implementations), so we cannot use Java8 as it is only available on Android API level 24 (Nougat)...

We will provide a RxJava2 wrapper of the SDK in the near future.

We appreciate your effort tho....

luciofm commented 7 years ago

@daniel-sc you could do a java8 module, like rocketchat-java8 and encapsulate the current SDK with CompletableFutures.

I know it is not ideal and it will be a lot of work, but this would be a nice addition to the SDK.

If you are willing to work on this, please take a look on the okhttp branch (https://github.com/RocketChat/Rocket.Chat.Java.SDK/pull/26), because we are doing some major work in there, renaming some Listeners to Callbacks (which make more sense, since they are one shot), Added error exceptions, and adding REST calls for some methods. we are approaching a 1.0-beta release, and after that the API should be stable, but right now it is changing a lot...

daniel-sc commented 7 years ago

Hi @luciofm

Thanks for the feedback - I already anticipated this situation, but needed to get this of my chest ;-)

As I don't have much time these days, so I'll let this rest (and maybe look into it once okhttp is merged).

I understand you want to stick with java6 because of android developers, but you should keep in mind this sdk is used as well in java 8 settings - there an additional lib such as RxJava2 would not make too much sense..

sacOO7 commented 7 years ago

Hey you have made a very good suggestion. I was gonna use CompletableFuture anyway, but due to compatibility issues we decided to keep it that way. Maybe you can write down patch for applications that want to use SDK in java8 based applications. Thank you once again !!!!!

sacOO7 commented 7 years ago

I think completableFuture allows returning only one parameter. We also need to have other params, like error. Otherwise it's impossible to proceed further and also need to handle all cases. Maybe it is better to have your patch without changing internal code, or disturbing functionality for which it is designed. We can have a thought on this . Thank you again !!!!

daniel-sc commented 7 years ago

Hi sachin!

You are right - CF can only return 1 param. But errors could be propagated as exceptions. Or - if they are not 'exceptional' - in some result object containing the potential error and a potential result.

I had second thoughts on RxJava as well: you'd probably need it even with java8 to create a smooth subscription interface (Observable).

Wrapping the existing API is of course a valid option - but a little messy and boring ;-) I'll probably write a java8 client from scratch - or at least a PoC. I'll share whenever there is something to show!

Am 14.09.2017 um 19:23 schrieb sachin shinde notifications@github.com:

I think completableFuture allows returning only one parameter. We also need to have other params, like error. Otherwise it's impossible to proceed further and also need to handle all cases. Maybe it is better to have your patch without changing internal code, or disturbing functionality for which it is designed. We can have a thought on this . Thank you again !!!!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

daniel-sc commented 7 years ago

Hi @luciofm, @sacOO7

you might want to check out https://github.com/daniel-sc/rocketchat-modern-client Of course this only covers few methods, but shows how the client should look like (from my perspective). As well it should be relatively easy to extend for further methods.

Best, Daniel

sacOO7 commented 7 years ago

Hey @daniel-sc looks like lot of promising code. I think we might add that kind of functionality in future. Thank you once again :+1:

luciofm commented 6 years ago

Closing, since we cannot support Java 8 as it was added in Android API 24.