jmnarloch / rxjava-spring-boot-starter

RxJava Spring MVC integration
Apache License 2.0
182 stars 46 forks source link

Remove completed boolean #5

Closed robertdanci closed 8 years ago

robertdanci commented 8 years ago

Hi,

Is there any reason for the complete flag?

It seemed to me superfluous so I deleted it. The tests pass fine.

Let me know what you think.

Regards, Robert

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.4%) to 83.908% when pulling 32fa24b26c89b2a75d6e2c6009a0ab7e03cf4f6d on robertdanci:master into 3796e310372921e24fccface5c14a9e1d849488a on jmnarloch:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.4%) to 83.908% when pulling d3c1d63ee597f2d6cfc393389ff1ecac83942587 on robertdanci:master into 3796e310372921e24fccface5c14a9e1d849488a on jmnarloch:master.

jmnarloch commented 8 years ago

So my guess is that I introduced the completed flag originally for covering the edge cases like actual completion, error or timeout, though I know that DeferredResult has the check to prevent from setting more the one value, though it uses explicit lock to do that.

At the moment I'm not sure if I want to remove the flag or not, both version seems to be correct at this point.

robertdanci commented 8 years ago

Some reasons to remove it:

  1. All unnecessary code should be removed as it creates confusion. Either we should write a test to prove the code is needed or remove it.
  2. In both instances DeferredResultSubscriber#onNext is used, it will only be called once: in SingleDeferredResult it is clear, in ObservableDeferredResult only once because you use observable.toList() which says in the documentation

    Returns an Observable that emits a single item, a list composed of all the items emitted by the source

jmnarloch commented 8 years ago

There one possible edge case to add in general: the illy implemented Subscriber, that calls the onCompleted method before executing onNext, though this probably might not happen in current scenario.

jmnarloch commented 8 years ago

Merged through https://github.com/jmnarloch/rxjava-spring-boot-starter/commit/1125a8e21c9a1c5e51e0c9de354dae52a6b488c3 Thanks!