Benne2jb code review2#5
Conversation
Bringing fork up to date
jabennett1515
left a comment
jabennett1515
left a comment
There was a problem hiding this comment.
Overall I felt that this app is really coming together, I few things that I had problems with, is that the app sometimes crashes when I login with facebook, I'm not sure if that is a problem with the spotify api. But whenever I login with facebook for spotify it closes the whole application. Maybe you haven't integrated anything with it yet, but I've noticed that.
So far though, it looks really nice. I think when you start to add more colors and to make the UI look a lot cleaner It will look like a very good app.
| val AUTH_CODE_REQUEST_CODE = 0x11 | ||
| lateinit var spotifyAppRemote: SpotifyAppRemote | ||
| private val AUTH_TOKEN_REQUEST_CODE = 0x10 | ||
| private val AUTH_CODE_REQUEST_CODE = 0x11 |
There was a problem hiding this comment.
I changed a lot of the variables to private as seen in the kotlin coding conventions website.
Reference: https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules
|
|
||
| val mOkHttpClient: OkHttpClient = OkHttpClient() | ||
| var mAccessToken: String? = null | ||
| private val mOkHttpClient: OkHttpClient = OkHttpClient() |
There was a problem hiding this comment.
I changed a lot of the variables to private as seen in the kotlin coding conventions website.
Reference: https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules
| val mOkHttpClient: OkHttpClient = OkHttpClient() | ||
| var mAccessToken: String? = null | ||
| private val mOkHttpClient: OkHttpClient = OkHttpClient() | ||
| private var mAccessToken: String? = null |
There was a problem hiding this comment.
I changed a lot of the variables to private as seen in the kotlin coding conventions website.
Reference: https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules
| private var mAccessToken: String? = null | ||
| private var mAccessCode: String? = null | ||
| var mCall: Call? = null | ||
| private var mCall: Call? = null |
There was a problem hiding this comment.
I changed a lot of the variables to private as seen in the kotlin coding conventions website.
Reference: https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules
| } | ||
|
|
||
| fun nextSong() { | ||
| private fun nextSong() { |
There was a problem hiding this comment.
I changed this function to private for more clarification.
Reference: https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules
| val songURL = songArtistArr?.get(0)?.replace(" ", "%20") | ||
| //artist = songArtistArr[1] + URL query additions | ||
| var artistURL = songArtistArr?.get(1)?.replace(" ", "%20") | ||
| val artistURL = songArtistArr?.get(1)?.replace(" ", "%20") |
There was a problem hiding this comment.
Here you didn't need "var" because val is immutable, it looks cleaner. to see more about it, check the references.
Reference: https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules
| val artistURL = songArtistArr?.get(1)?.replace(" ", "%20") | ||
| //songArtistURL concatenates songUrl and artistURL except inbetween the two there are query params | ||
| var songArtistURL = "$songURL%2C%20$artistURL" | ||
| val songArtistURL = "$songURL%2C%20$artistURL" |
There was a problem hiding this comment.
Here you didn't need "var" because val is immutable, it looks cleaner. to see more about it, check the references.
Reference: https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules
| object RetrofitClientInstance { | ||
| private var retrofit: Retrofit? = null | ||
| private val BASE_URL = "https://api.spotify.com/v1/" | ||
| private const val BASE_URL = "https://api.spotify.com/v1/" |
There was a problem hiding this comment.
This variable is never being changed, so it will seem to make more sense that it is a const variable.
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:text="pause" | ||
| android:text="Pause" |
There was a problem hiding this comment.
Changing to capital P, because it looks cleaner.
There was a problem hiding this comment.
I'd take this two steps further:
- Extract to strings.xml, so that a) you'll have all of your String resources in one place, and b) better for internationalization, as you can have a separate strings.xml for each language/
- Use an ImageButton. Pictures are faster than words.
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:text="search" | ||
| android:text="Search" |
There was a problem hiding this comment.
Changing to capital S, because it looks cleaner. Small UI fix.
|
Several good changes that align well with Kotlin coding conventions, and reduce technical debt. I recommend merging this branch. |
2 participants
I thought the project was a great idea and that it's unique how you're using it for parties. I would love to actually use this app for when I have friends over and then they will be able to request songs and we can all listen to what everyone likes.
What I learned from this app: