-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
private val BY_TAG = "tag.gettoptracks" | ||
private val BY_CHART = "chart.gettoptracks" | ||
|
||
override fun onCreate(savedInstanceState: Bundle?) { |
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 2 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.
for button text, maybe use @string so that it would be easier to reuse the same text. other than that 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.
Really Nice code 👍
There are some minor doc changes to do, but it should take seconds to solve.
import android.widget.ArrayAdapter | ||
import android.widget.ListView | ||
import ch.sdp.vibester.api.LastfmHelper | ||
|
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.
Some javadoc to describe the class would be great. Maybe the method too.
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.
Then it's temporary so not mandatory
import org.json.JSONArray | ||
import org.json.JSONObject | ||
import java.lang.IllegalArgumentException | ||
|
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.
Maybe adding a class doc ?
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.
What does a song List represent. Also not a List. Doc may be important
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 Javadoc looks good now
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 work 👍
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.
Looks good to me. The code is well-documented.
Code Climate has analyzed commit dcb4e45 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 97.1% (80% is the threshold). This pull request will bring the total coverage in the repository to 94.4% (0.3% change). View more on Code Climate. |
LastfmGenre API
This PR is related to issue #70
This PR contains:
ch.sdp.vibester.api: LastfmApi: an api with a function querySongList(...) that will return list of songs within a given tag/genre
ch.sdp.vibester.api: LastfmHelper: a helper to combine songs from two Lastfm queries. This is done to have random songs each game.
ch.sdp.vibester.api: LastfmUri: a data class to build Uri to lastfm queries
ch.sdp.vibester.model: SongList: a class that represents an object retrieved from lastfm query
ch.sdp.vibester: GenreTemporary: an activity that is used to test the Genre API
In addition, the code is tested with a coverage of: [92]%