Feature/pr task #1
Conversation
| implementation(libs.retrofit.kotlinx.serialization.converter) | ||
| implementation("com.squareup.okhttp3:logging-interceptor:4.12.0") |
There was a problem hiding this comment.
This should be moved to libs.versions.toml
| @@ -4,13 +4,18 @@ import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFact | |||
| import kotlinx.serialization.ExperimentalSerializationApi | |||
| import kotlinx.serialization.json.Json | |||
| import okhttp3.MediaType | |||
| private val json = Json { | ||
| ignoreUnknownKeys = true | ||
| isLenient = true |
There was a problem hiding this comment.
This is crucial change and it's not directly connected with "logging".
Why do we need lenient mode?
| import retrofit2.Retrofit | ||
|
|
||
| class ApiService { | ||
|
|
||
| private val contentType = MediaType.get("application/json") | ||
| private val contentType = "application/json".toMediaType() | ||
| private val client = OkHttpClient.Builder().addInterceptor(HttpLoggingInterceptor().setLevel(HttpLoggingInterceptor.Level.BODY)).build() |
There was a problem hiding this comment.
This interceptor logs for any build type - it shouldn't log in release build
There was a problem hiding this comment.
#out-of-pr
ApiService - there are few things we can improve here :)
- Strings (
"application/json", "https://itunes.apple.com/") could be extracted to consts ApiServicecould be anobject- there's no point to create instances of this class other than obtainingItunesService- Properties like
contentType,client,jsonare only required to initializeserviceproperty.
I'd suggest to wrap it into methods and delegate service creation, e.g.:
val service: ItunesService = createItunesService()
private fun createItunesService() = {
val httpClient = getHttpClient()
val retrofit = getRetrofit(httpClient)
return retrofit.create(ItunesService::class.java)
}
OR
We should consider using DI framework in order to provide such dependencies to consumers :)
Either way we should discuss it and do it in a separate task.
| var selectedSong: Song? by remember { mutableStateOf(null) } | ||
| if (!detailsClicked) { | ||
| Column(Modifier.verticalScroll(rememberScrollState())) { | ||
| songs.forEach { |
There was a problem hiding this comment.
I'm assuming songs can contain many items - LazyColumn will be a better component than Column (performance)
| } else { | ||
| Column(modifier = Modifier.fillMaxWidth()) { | ||
| selectedSong?.let { | ||
| Text("What a great detalis page!") |
There was a problem hiding this comment.
"What a great detalis page!" should be moved to string resources
| } | ||
| } | ||
| Button(onClick = { detailsClicked = false }) { Text("Back") } |
There was a problem hiding this comment.
"Back" should be moved to string resources
There was a problem hiding this comment.
I'd suggest to extract parts of the layout to separate functions, to provide more readable structure.
E.g.:
fun SongListScreen(){
if(!detailsClicked){
SongList()
} else {
SongDetails()
}
}
fun SongItem(){
}| @@ -2,5 +2,6 @@ package com.tooploox.androidrecruitmenttask | |||
|
|
|||
| data class Song( | |||
| val title: String, | |||
| val artist: String | |||
| val artist: String, | |||
| val releaseYear: Int, | |||
|
I will add a summary here and some information about pr itself: PR:
Summary:
|
|
Oh, two more things popped up:
|
Please review this PR at your your leisure. We expect this review to take under 30 minutes, but feel free to take as much or as little time as you want.
Focus on all the areas of code quality you feel important, treat it as a code a colleague in your project pushed to a repo you contribute to.