search

mkoslacz / Moviper

A Mosby based VIPER library for Android
Apache License 2.0
77 stars 9 forks source link

Add new Sample and RxSample #1

Closed jakubjodelka closed 7 years ago

mkoslacz commented 7 years ago

I have written some comments inline. Wrap your lines and extract strings to constants please.

jakubjodelka commented 7 years ago

fixed.

mkoslacz commented 7 years ago
  1. UserViewHolder should be extracted to separate file.
  2. UserListAdapter shouldn't have a list in its constructor, it should be passed through a setter.
  3. Stick to the choice you made — UserViewHolder should have views injected using Butterknife.
  4. The UserAdapter's getItemCount method can produce NPE.
  5. UserAdapter has reference to context and you are not nullying it on Activity's onDestroy — there will be a memory leak.
  6. You should have a static starter method on each activity you start. Routing should just invoke this method. If you have X activities that start activity A, you will be duplicating the code X times on each Routing that starts mentioned activity A.
  7. You pass UserViewHolder to the ListingRouting's startUserDetailsActivity method and do nothing with that.
  8. You use ViewHelper in FullScreenPhotoActivity and do nothing with that.
  9. You use the Android framework methods in UserDetailsPresenter. That should be moved to the Routing. Moreover, it's not very efficient. Actually, it will be much better to create own bundle with the data you want to display instantly in UserDetailsActivity and user id / name to fetch the rest of the data.

Fix mentioned problems in both samples please.