-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
@GET("https://ws.audioscrobbler.com/2.0/") | ||
fun getSongList(@QueryMap paramsMap: MutableMap<String, String>): Call<Object>; | ||
|
||
companion object { |
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.
We can extract a general retrofit interface from LastfmApi and LyricsOVHApi to eliminate this duplication
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.
Thanks for all the work you have done! It took me quite a while even to review this PR.
Good job. I leave some comments but it's you to decide if you want to change.
setContentView(R.layout.activity_end_basic_game_temporary) | ||
|
||
var bundle = intent.extras | ||
var txtView = findViewById<TextView>(R.id.score) |
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 think we need to have a naming convention for the ids, something like activityNameActualName.
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.
Yes we have to refactor all our id's in the future
import org.json.JSONObject | ||
import retrofit2.Call | ||
import retrofit2.Callback | ||
import retrofit2.Response | ||
|
||
/** | ||
* Activity to show the list of songs for a chosen tag | ||
*/ | ||
class GenreTemporary : AppCompatActivity() { | ||
private val BY_TAG = "tag.gettoptracks" |
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.
Will it be a good idea to store these val as constant within lastFM API? 🧐 We might need them somewhere else.
app/src/main/java/ch/sdp/vibester/activity/TypingGameActivity.kt
Outdated
Show resolved
Hide resolved
@GET("https://ws.audioscrobbler.com/2.0/") | ||
fun getSongList(@QueryMap paramsMap: MutableMap<String, String>): Call<Object>; | ||
|
||
companion object { |
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.
We can extract a general retrofit interface from LastfmApi and LyricsOVHApi to eliminate this duplication
onView(withId(R.id.songsListView)).check(matches(isDisplayed())) | ||
Thread.sleep(1000) // wait for data to fetch | ||
onData(allOf(`is`(instanceOf(String::class.java)))).atPosition(2).perform(click()) | ||
Thread.sleep(1500) |
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.
it might be cleaner to set a const for 1500 since you use it many times
@@ -46,7 +46,7 @@ class AuthenticationActivityTest { | |||
onView(withId(R.id.username)).perform(ViewActions.typeText(username), closeSoftKeyboard()) | |||
onView(withId(R.id.password)).perform(ViewActions.typeText(password), closeSoftKeyboard()) | |||
onView(withId(R.id.logIn)).perform(click()) | |||
Thread.sleep(3_000) | |||
Thread.sleep(5_000) |
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.
same here for 5000
@@ -40,76 +40,76 @@ class LyricsBelongGameActivityTest { | |||
fun clean() { | |||
Intents.release() | |||
} | |||
|
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.
My bad. This function can be deleted
Co-authored-by: jiabaow <[email protected]>
Co-authored-by: jiabaow <[email protected]>
Co-authored-by: jiabaow <[email protected]>
Code Climate has analyzed commit 91c5998 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 92.8% (80% is the threshold). This pull request will bring the total coverage in the repository to 91.2%. 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.
Nice work !! 👍 Some docs to add when you've got time
setContentView(R.layout.activity_end_basic_game_temporary) | ||
|
||
var bundle = intent.extras | ||
var txtView = findViewById<TextView>(R.id.score) |
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.
Yes we have to refactor all our id's in the future
|
||
/** | ||
* Activity to show the list of songs for a chosen tag | ||
*/ | ||
class GenreTemporary : AppCompatActivity() { | ||
private val BY_TAG = "tag.gettoptracks" | ||
private val BY_CHART = "chart.gettoptracks" | ||
private val BY_ARTIST = "artist.gettoptracks" | ||
private val baseurl = "https://ws.audioscrobbler.com/2.0/" |
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.
why baseurl is not BASEURL ? not const ?
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 have missed the comment. I will add it in my other pr.
@@ -20,6 +21,7 @@ private const val REQUEST_AUDIO = 100 | |||
|
|||
class LyricsBelongGameActivity : AppCompatActivity() { | |||
private lateinit var speechInput : String | |||
private val baseUrl = "https://api.lyrics.ovh/" |
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.
same here: Shouldn't be BASEURL ?
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 have missed the comment. I will add it in my other pr.
@@ -20,6 +21,7 @@ private const val REQUEST_AUDIO = 100 | |||
|
|||
class LyricsBelongGameActivity : 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.
What does the classs represent ? 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.
It's my task for this week, I will add it in my PR
import retrofit2.Retrofit | ||
import retrofit2.converter.gson.GsonConverterFactory | ||
|
||
object ServiceBuilder { |
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.
Doc ? what is this object for ?
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 have missed the comment. I will add it in my other pr.
This PR is relalted to issue #93
Now the user can choose the genre/artist and play a solo game. The music is playing, the user types the song, and gets the score. In the end, the score is shown.
This PR contains:
ch.sdp.vibester.helper: GameManager: a class that manages the solo game for chosen tag
ch.sdp.vibester: GenreTemporary: update an activity with new buttons for game
ch.sdp.vibester: EndBasicGameTemporary: temporary to show score after the game
ch.sdp.vibester.activity: TypingGameActivity: updated activity to play a solo game of 10 song
There were also small updates in lastfmpi files to fit the solo game.
All the added functionality is tested, the coverage is [92.7%]