-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
jiabaow
commented
Mar 31, 2022
•
edited
Loading
edited
- refactor API service builder
- refactor Lastfm method
- reformat code in all files (based on android studio suggestions) [sorry guys, I should have done this in a separate PR...]
@@ -53,28 +50,29 @@ class GenreTemporary : AppCompatActivity() { | |||
startActivity(newIntent) | |||
} | |||
|
|||
// view is needed, remove it will result failure in tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 5 locations. Consider refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function call with different parameters, cannot really refactor
} | ||
|
||
fun playBillieEilish(view: View){ | ||
performQuery(LastfmUri(method = BY_ARTIST, artist = "Billie Eilish")) | ||
fun playBillieEilish(view: View) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 5 locations. Consider refactoring.
@@ -26,7 +26,7 @@ import kotlin.random.Random | |||
*/ | |||
@RunWith(AndroidJUnit4::class) | |||
class AuthenticationActivityTest { | |||
private val SLEEP_TIME: Long = 5000 | |||
private val sleepTime: Long = 5000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use camel case for all the constants from now on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will say yes but only when they are inside a class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job on refactoring everything, the code feels much more legible! 👍
Code Climate has analyzed commit 6a15331 and detected 6 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 85.6% (80% is the threshold). This pull request will bring the total coverage in the repository to 90.9% (-0.3% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have much to add. The code looks better after refactoring. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks for the refactoring!