topcoderinc / va-kidney-android

2 stars 3 forks source link

Code Quality Issues #42

Closed talesforce closed 6 years ago

talesforce commented 6 years ago

1) There are multiple cases where there are uppercase package names(i.e CustomView, Util, Model etc), this is not a recommended standard, please can we rename them to recommended lowercase naming?

2) Classe are not properly formatted, indentation, missing whitespace etc

4) BaseAdapter is not an ideal choice for handling list data and its recommended to use RecyclerView.Adapter (especially when BaseAdapter implementation is not even using ViewHolder pattern, without ViewHolder pattern there will be performance issues that will impact in the smoothness of scrolling lists, this might get raised when used by multiple users who will start to pick up issues with lists in the App)

5) Why use an entire Third-Party library(com.makeramen:roundedimageview:2.3.0) to have a circular image, when the App already has Glide which has image transforms that can make the image circular when loading it into ImageView.

talesforce commented 6 years ago

Please follow http://apps.topcoder.com/forums/?module=Thread&threadID=919510&start=0&mc=14 for a detailed discussion

fanazhe commented 6 years ago

Please confirm 3. BaseAdapter need to be replaced by RecyclerView.Adapter and 4. com.makeramen:roundedimageview need to be replaced by Glide crop method.

fanazhe commented 6 years ago

Is there anyone can reply and confirm? This challenge doesn't have too much time remaining now.

talesforce commented 6 years ago
  1. Confirmed.
fanazhe commented 6 years ago

@talesforce How about point 4? Must the com.makeramen:roundedimageview be replaced by Glide crop method?

KevinLeigh commented 6 years ago

@fanazhe @talesforce Yes I believe we should use Glide for making image circular using transforms. I have stumbled across a crash in the App because of an OutOfMemory error caused from RoundedImageView third party library.

screen shot 2018-06-17 at 07 51 03
KevinLeigh commented 6 years ago

Also believe some images do not show.

talesforce commented 6 years ago

@fanazhe - Yes, please use Glide

KevinLeigh commented 6 years ago

@talesforce Hey there,

A third party library for sqlite data persistence like Sugar (https://github.com/chennaione/sugar) is not really recommended.

We should rather be using Room which is an official Android library please see https://developer.android.com/topic/libraries/architecture/room. Room is much more simpler than Sugar and has much more power/flexibility with capabilities i.e like being able to observe changes to DB with LiveData and automatically have the view reflect the DB changes. (kinda like the older Android CursorLoader/CursorAdapter but more modern)

Room probably has more test coverage than Sugar(75%) as well.

Here is a nice tutorial on Room: https://codelabs.developers.google.com/codelabs/android-persistence/#0

talesforce commented 6 years ago

Hi @KevinLeigh - Thanks for the note. Since we're quiet close to submission deadline, we'll not be enforcing any more library changes for now. If required, we'll deal with them after this challenge.

KevinLeigh commented 6 years ago

@talesforce All good, maybe next iteration. Also do not mind still adding issues or better approaches as I go along.

lakshmansai commented 6 years ago

Is implementing view holder pattern enough cause Recycler view in Goals Fragment has a bug like scrolling through the goals fills the Arc progress.

kofoworola commented 6 years ago

@lakshmansai i also noticed this, but i guess we go with this due to deadline

talesforce commented 6 years ago

@lakshmansai @kofoworola - Yes, that's fine. We'll deal with it later.