Kata feedback

[akarnokd]

I took the challenge:

listOnly3rdAnd4thCountry

Lists are 0 based thus the 3rd item is at index 2 yet the matching test expects index 3 and 4.

getCurrencyUsdIfNotFound

The expected format for the default value should be defined upfront, I thought the default response should be "Usd" but the test expects "Usd (Default)"

isPopulationMoreThan1Million

The method name doesn't indicate that the user should check if all countries in the list have more than 1M population, the Single<Boolean> result type may also indicate to check if any of the countries is over 1M.

rx_ListPopulationMoreThanOneMillion_FutureTask

It is reasonable to expect that when the user sees Future or FutureTask he/she will use fromFuture(Future, Scheduler) to make sure any potential blocking on Future.get() doesn't block the subscribing thread. The test is not ready for this and may fail the test.

rx_CountryNameInCapitalsWithNPE

RxJava 2 is largely a non-null library and nothing indicates the Country.name may be null. In addition, the test expects a particulare string to be returned for the null name.

[sergiiz]

Thanks a lot for the feedback @akarnokd.
I removed NPE case from tests, will think about more correct example here. All other issues should be resolved now, please confirm if we can close the ticket.

[akarnokd]

listPopulationMoreThanOneMillion:
no need for the Scheduler argument, just make sure the unit test uses TestObserver.awaitTerminalEvent before checking the value returned.

[sergiiz]

Thanks @akarnokd, awaitTerminalEvent added to the test

[vnickolov]

Hi @sergiiz I am going to use this thread instead of opening a new one. Thank you very much for taking the time to create this, it was useful exercise.
I forked it and updated to RxJava 3, JUnit 5 and Gradle 5.x today, you can review that if you're interested, there are some changes in RxJava specifically and awaitTerminalEvent doesn't exist anymore for instance.

Thank you again!

[sergiiz]

Thank you @vnickolov! would love to have your update to be merged to this repo!