feature branch commit #1
Conversation
add app module changes
| import kotlinx.serialization.Serializable | ||
|
|
||
| data class ShopListResponse( | ||
| val list_id: String, |
There was a problem hiding this comment.
here list_id must be listId
please follow the camel case for local variable, to overcome this issue you can use @SerializedName("list_id") Gson Annotation to specify the variables comes from the api side.
|
|
||
| override suspend fun getShopListItems(listId: String): List<ShopListItemResponse> = | ||
| coroutineScope { | ||
| Thread.sleep(2) |
There was a problem hiding this comment.
use delay() couroutines function for delay instead of Thead.sleep()
| @@ -0,0 +1,10 @@ | |||
| package com.netguru.codereview.network | |||
|
|
|||
| class ShopListRepository(private val shopListApi: ShopListApi) { | |||
There was a problem hiding this comment.
(this is not a must, I just put this comment with comparing to clean code architecture with MVVM)
there is no domain layer. its better to have a interface called ShopListRepository inside the repository list folder in domain folder.
in the network data layer you can use implementation class of it "ShopListRepositoryIml"
|
|
||
| import com.netguru.codereview.network.model.ShopListItemResponse | ||
|
|
||
| class ShopList( |
There was a problem hiding this comment.
make this class int to a data class.
| super.onViewCreated(view, savedInstanceState) | ||
| viewModel = ViewModelProvider(this).get(MainViewModel::class.java) | ||
|
|
||
| viewModel!!.shopLists.observe(this, { lists -> |
There was a problem hiding this comment.
change the owner of view model to 'viewLifecycleOwner' since it represent the fragmentview`s lifecycle.
There was a problem hiding this comment.
move lambda argument out of the parentheses
| app:layout_constraintTop_toBottomOf="@+id/title" /> | ||
|
|
||
| <ProgressBar | ||
| android:id="@+id/message" |
There was a problem hiding this comment.
name must be meaning full. use name like progress_bar
| getUpdateEvents() | ||
| } | ||
|
|
||
| fun events(): LiveData<String> = eventLiveData |
There was a problem hiding this comment.
no need to pass LiveData with function here. just observe the eventLiveData filed from fragment. remove function here. and make eventLiveData public.
|
|
||
| class MainViewModel : ViewModel() { | ||
|
|
||
| private val shopListRepository = ShopListRepository(ShopListApiMock()) |
There was a problem hiding this comment.
add this repository into the constructor in MainViewModel.
private val shopListRepository: ShopListRepository = ShopListRepository(ShopListApiMock())
|
|
||
| suspend fun getShopListItems(listId: String) = shopListApi.getShopListItems(listId) | ||
|
|
||
| fun updateEvents() = shopListApi.getUpdateEvents() |
There was a problem hiding this comment.
name is not meaning full. function is returing data. not update events
| buildTypes { | ||
| release { | ||
| minifyEnabled true | ||
| minifyEnabled false |
There was a problem hiding this comment.
put minifyEnable true in release mode only. create separate debug mode for development (security concern)
| data class ShopListItemResponse( | ||
| val itemId: String, | ||
| val name: String, | ||
| val quantity: Double |
There was a problem hiding this comment.
quantity must be a Integer
| class MainFragment : Fragment() { | ||
|
|
||
| @Inject | ||
| private var viewModel: MainViewModel? = null |
There was a problem hiding this comment.
use lateInit and make MainViewModel not null ( remove ?)
and you can remove force null check on line number 36, and 52
| latestIcon?.load(it.first().iconUrl) | ||
| } | ||
|
|
||
| progressBar?.isVisible = false |
There was a problem hiding this comment.
make progress bar visible in init state.
this line is not working, use visibility parameter for visibility change.
progressBar?.visibility = View.GONE
|
|
||
| <androidx.recyclerview.widget.RecyclerView | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="match_parent" |
There was a problem hiding this comment.
no id for the recyclerview
| viewModel = ViewModelProvider(this).get(MainViewModel::class.java) | ||
|
|
||
| viewModel!!.shopLists.observe(this, { lists -> | ||
| val progressBar = view.findViewById<ProgressBar>(R.id.message) |
There was a problem hiding this comment.
widget fields must be class scope and should not init inside observe functions. this assigin must be out side the observe function and inside onViewCreated
There was a problem hiding this comment.
val latestIcon = view.findViewById(R.id.latest_list_icon) should be the same above
| list.list_id, | ||
| list.userId, | ||
| list.listName, | ||
| list.listName, |
There was a problem hiding this comment.
lastName passes to ShopList -> iconUrl filed
invalid data
| # debugging stack traces. | ||
| #-keepattributes SourceFile,LineNumberTable | ||
|
|
||
| # If you keep the line number information, uncomment this to |
There was a problem hiding this comment.
specify the excluded classes list here
| val progressBar = view.findViewById<ProgressBar>(R.id.message) | ||
| val latestIcon = view.findViewById<ImageView>(R.id.latest_list_icon) | ||
|
|
||
| val shopLists = lists.map { mapShopList(it.first, it.second) }.also { |
There was a problem hiding this comment.
here application logics and model conversons should not inside the fragment or activities. keep them inside the viewModel.
(move "mapShopList" function inside to the viewModel)
icon set Url also must be inside the ViewModel
add app module changes