search

VictorAlbertos / RxActivityResult

A reactive-tiny-badass-vindictive library to break with the OnActivityResult implementation as it breaks the observable chain.
Apache License 2.0
593 stars 72 forks source link

Review #18

Closed akarnokd closed 8 years ago

akarnokd commented 8 years ago

Hi, just looking at libraries using RC2 and found these issues.

The emitted field is private and generates an accessor:

https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/ActivitiesLifecycleCallbacks.java#L57

The same emitted field is not volatile and possibly read in another thread:

https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/ActivitiesLifecycleCallbacks.java#L70

These fields are also private and generate accessors:

https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/ActivitiesLifecycleCallbacks.java#L12

liveActivityOrNull is possibly read in another thread and should be volatile:

https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/ActivitiesLifecycleCallbacks.java#L63

subject could be final:

https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/RxActivityResult.java#L52

clazz is private and generates an accessor:

https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/RxActivityResult.java#L51 https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/RxActivityResult.java#L100 https://github.com/VictorAlbertos/RxActivityResult/blob/2.x/rx_activity_result/src/main/java/rx_activity_result/RxActivityResult.java#L138

VictorAlbertos commented 8 years ago

Hi @akarnokd.

I understand your comments about making some fields volatile and final. But I don't follow you when you say that certain fields are private and still they generate "accessors".

akarnokd commented 8 years ago

Jake Wharton had a talk recently about private fields and methods increasing the method count due to VM requirements.

VictorAlbertos commented 8 years ago

Thanks :) I'll remove the modifier.