-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
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.
Nice works 👍 , just some small easy changes that can be done now
|
||
class DownloadActivity : AppCompatActivity() { |
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 add documentations
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.
for all public methods of class (except override)
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.
Done!
fun switchToDownload(view: View) { | ||
sendDirectIntent(DownloadActivity::class.java) | ||
} | ||
|
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 be easy to test, currently untested
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.
Done!
onView(withId(R.id.download_songName)).perform(typeText(songName), closeSoftKeyboard()) | ||
Thread.sleep(100) | ||
onView(withId(R.id.download_downloadsong)).perform(click()) | ||
Thread.sleep(2000) |
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.
Try using constants for the Thread.sleep calls instead of hardcoded values.
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.
Done!
if(extract.exists()) { | ||
assert(false) | ||
} else { | ||
assert(true) |
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.
Simpler to use assert(extract.exists()) here.
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.
Actually, for that one it's assert(!extract.exists()) due to it being a negative assumption. Otherwise you are correct, thanks!
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.
Done!
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.
The code looks good to me, there are just some minor issues to be refactored in the tests (see comments).
Code Climate has analyzed commit 6c9bebf and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 83.9% (80% is the threshold). This pull request will bring the total coverage in the repository to 91.1% (-0.5% 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.
Top 👍
Implemented the ability to download song extracts, acquired from the iTunes API.