Project review #2
Conversation
| package com.example.compose | ||
|
|
||
| import android.content.Context.MODE_PRIVATE | ||
| import androidx.compose.ui.test.* |
There was a problem hiding this comment.
Don't use wildcard imports by default. You can set it in android studio settings to not use wildcard imports
There was a problem hiding this comment.
why should not use wildcard?
when we enable isMinifyEnabled , then it will remove all codes which we won't need. is it right?
There was a problem hiding this comment.
Wildcard import increase build time. It's not significant, but if you would work with application containing 1000 classes the overhead gets bigger. You have to build project every time you want to test your changes, so every developer is doing few builds per day. In big projects it has real impact and it affect all developers.
Minify is totally different - enabling minify increase build time, but reduce size of final APK. For more information and details about minify and code shrinking check https://developer.android.com/studio/build/shrink-code
There was a problem hiding this comment.
Also using minify in multidex complex application with multiple dependencies might not be free - It might require more customisation than just setting isMinifyEnabled to true
There was a problem hiding this comment.
I did not notice that it will increase the build time.
| <meta-data | ||
| android:name="android.app.lib_name" | ||
| android:value="" /> |
There was a problem hiding this comment.
Is this required?
There was a problem hiding this comment.
I do not think so. I will remove it.
| .readTimeout(60, TimeUnit.SECONDS) | ||
| .writeTimeout(60, TimeUnit.SECONDS) |
There was a problem hiding this comment.
It's a debatable issue - 60 seconds of general timeout seems huge, like 4-6 times too big - if you are blocking some action on screen during remote API call execution, then this mean you can block user screen for 60 seconds - it's better to fail sooner, i.e. after 15 seconds and then let user retry/go back to different screen/feature/etc.
| OkHttpClient.Builder() | ||
| .readTimeout(60, TimeUnit.SECONDS) | ||
| .writeTimeout(60, TimeUnit.SECONDS) | ||
| .addInterceptor(logging) |
There was a problem hiding this comment.
Learning - read a little bit about interceptors. You can do a lot of useful stuff with custom interceptors :)
There was a problem hiding this comment.
I have a question maybe related to interceptors. envisage, an api return different response type for the same request, how can we handle it with retrofit?
The ok response when I have permission:
{ method: { _content: "flickr.test.echo" }, format: { _content: "json" }, api_key: { _content: "8cb5d1fd54f34b91543e5968a1ae9cde" }, stat: "ok" }
The same request return fail response, when I do not have permission:
{ stat: "fail", code: 99, message: "Insufficient permissions. Method requires read privileges; none granted." }
There was a problem hiding this comment.
In short: Values returned by retrofit should be wrapped. One of wrappers is Response class. It contain method isSuccessful. So whenever you get a response you can do something like:
If (response.isSuccessful) { do something based on response.body value } else { display error message to the user, unlock the view if you are using custom spinner/view locker }
Write me on teams if you want to talk this through
There was a problem hiding this comment.
Here is some short example - someone is trying to get QuoteList via retrofit. At the end of tutorial there is response of type Response
https://www.geeksforgeeks.org/retrofit-with-kotlin-coroutine-in-android/
| const val DATABASE_NAME = "app_database" | ||
| const val FAVORITE_TABLE_NAME = "favorite_rates" |
There was a problem hiding this comment.
Huge plus for adding const val and reusing them in different classes
| */ | ||
|
|
||
| abstract class BaseViewModel<T : UIState, E : UIEvent>(initialState: T) : ViewModel() { | ||
| private var internalSate: MutableState<T> = mutableStateOf(initialState) |
There was a problem hiding this comment.
could be val - you are creating one object of type MutableState during initialisation. Later on setState method just change underlying value in object - it's still the same MutableState but with different value
I will add here some comments. If you want to resolve them, please create separate PR with resolution proposition.