Conversation
| buildTypes { | ||
| release { | ||
| minifyEnabled true | ||
| minifyEnabled false |
There was a problem hiding this comment.
minifyEnabled should be left to true. This allows R8 to shrink the apk size and optimize the app. This also improves the security of the app through the obfuscation of code
| import kotlinx.serialization.Serializable | ||
|
|
||
| data class ShopListResponse( | ||
| val list_id: String, |
There was a problem hiding this comment.
Please change variable name list_id to listId. Camel case should always be preserved even for classes that map the response. In order to preserve compatibility with the JSON please use the @SerialName annotation (e.g. @SerialName("list_id"))
| class MainFragment: Fragment() { | ||
|
|
||
| @Inject | ||
| private var viewModel: MainViewModel? = null |
There was a problem hiding this comment.
Please inject viewmodel lazily by using viewModels(): private val viewModel: MainViewModel by viewModels()
| @@ -1,21 +1,2 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
There was a problem hiding this comment.
Please revert this change. Application and activities need to be declared inside the manifest file.
|
|
||
| override suspend fun getShopListItems(listId: String): List<ShopListItemResponse> = | ||
| coroutineScope { | ||
| Thread.sleep(2) |
There was a problem hiding this comment.
Never use Thread.sleep. Use delay() instead. This suspends the current coroutine without blocking the thread.
| @@ -0,0 +1,10 @@ | |||
| package com.netguru.codereview.network.model | |||
There was a problem hiding this comment.
Move this inside the shoplist module
| @@ -0,0 +1,11 @@ | |||
| package com.netguru.codereview.ui.model | |||
There was a problem hiding this comment.
Move it in the appropriate module.
| android:id="@+id/latest_list_icon" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| app:layout_constraintTop_toBottomOf="@+id/title" |
There was a problem hiding this comment.
Minor point: better to use @id/title than @+id/title as it improves id checking.
| @@ -0,0 +1,10 @@ | |||
| package com.netguru.codereview.network | |||
|
|
|||
| class ShopListRepository(private val shopListApi: ShopListApi) { | |||
There was a problem hiding this comment.
It would be better to split the repository into an interface ShopListRepository and an implementation say ShopListRepositoryImpl in order to prevent the leaking of implementation details and to improve code testability
| android:layout_width="wrap_content" | ||
| android:layout_height="match_parent" | ||
| app:layout_constraintTop_toBottomOf="@id/latest_list_icon" | ||
| app:layout_constraintBottom_toBottomOf="@id/message" |
There was a problem hiding this comment.
Setting constraints to something like a ProgressBar that can often change its visibility is not recommended.
1 participant
No description provided.