WF todo app for CoderSchool review

[phamquangsang]

Here is my complete WF todo App. Please review. @coderschoolreview

[coderschoolreview]

Thank you for your Android submission, phamquangsang!

:ice_cream: Everything looks good with the format of your submission. We'll be reviewing your submission soon!

[hoanle]

Hi @phamquangsang

Thanks for submission

In general

• The app could not launch due to the crash java.lang.ClassCastException: android.graphics.drawable.ColorDrawable cannot be cast to android.support.v7.widget.RoundRectDrawableWithShadow for CardView, you should use setCardBackgroundColor instead of setBackgroundColor
• All required features are done nicely.
• Interesting layout. The swipe to delete action is cool
• It's very nice that you use SQLiteDatabase, your custom adapter, fragment and cardview

Suggestion

• You could make the code clearer and cleaner by shortening functions. Some are too long (for example onCreate of TodoDatabaseHelper)
• I understand that you divide the project into module (like DetailListActivity, ToDoListActivity). My suggestion, it's more common that the project is arranged by type of class. In such way, adapter types should be in the same package, activity class should be in the same package.
  • For swipe to delete item
• Action 1: Using AsyncTask to remove item from SQLite database
• Action 2: Removing item from adapter

What if 1 fails & 2 succeeds? The item is still in database but has been removed from the list?

• It's better to limit the logic implementation on onBindViewHolder, it should simply do the binding view action
• You seems to like AsyncTask a lot. It's some kind of cool doing work in background thread. Be careful because it's very easy to have memory leak, database lock, update UI error and many other drawback. A good read dark side of asynctask

[phamquangsang]

Hi @hoanle thank you for your feedback and suggestions.

• I didn't test the app in pre-Lollipop version, my bad. I fixed the bug relate to change background color and committed to git hub.
• For swipe to delete item: I've improved data integrity; Action 1 and 2 now run on the same thread, from now delete item from Adapter's data set only if delete item from database is succeed.
• I tried not to access to database in UI thread to get rid of "skipped xx frames! the application may doing to much work in its main thread". Thank to asyntask, the number of frames skipped always less than 50 so the UI should be smooth and responsive. But you was absolutely right about asycntask's drawbacks. it is really difficult for me to control the logic flow of the application, and the article you gave me really helpful; it surprised me, memory leak, update UI error, life circle... I never knew it.

On Sun, Feb 28, 2016 at 9:51 PM hoanle notifications@github.com wrote:

Hi @phamquangsang https://github.com/phamquangsang

Thanks for submission

In general

• The app could not launch due to the crash java.lang.ClassCastException: android.graphics.drawable.ColorDrawable cannot be cast to android.support.v7.widget.RoundRectDrawableWithShadow for CardView, you should use setCardBackgroundColor instead of setBackgroundColor
• All required features are done nicely.
• Interesting layout. The swipe to delete action is cool
• It's very nice that you use SQLiteDatabase, your custom adapter, fragment and cardview

Suggestion

• You could make the code clearer and cleaner by shortening functions. Some are too long (for example onCreate of TodoDatabaseHelper)
• I understand that you divide the project into module (like DetailListActivity, ToDoListActivity). My suggestion, it's more common that the project is arranged by type of class. In such way, adapter types should be in the same package, activity class should be in the same package.
  • For swipe to delete item
• Action 1: Using AsyncTask to remove item from SQLite database
• Action 2: Removing item from adapter

What if 1 fails & 2 succeeds? The item is still in database but has been removed from the list?

-

It's better to limit the logic implementation on onBindViewHolder, it should simply do the binding view action

You seems to like AsyncTask a lot. It's some kind of cool doing work in background thread. Be careful because it's very easy to have memory leak, database lock, update UI error and many other drawback. A good read dark side of asynctask http://bon-app-etit.blogspot.com/2013/04/the-dark-side-of-asynctask.html

— Reply to this email directly or view it on GitHub https://github.com/phamquangsang/WFTodo2/issues/1#issuecomment-189885640 .