Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Jwen/enhance lyric game #187

Merged
merged 40 commits into from
Apr 18, 2022
Merged

Jwen/enhance lyric game #187

merged 40 commits into from
Apr 18, 2022

Conversation

jiabaow
Copy link
Collaborator

@jiabaow jiabaow commented Apr 18, 2022

Screen Shot 2022-04-18 at 13 08 35

What is done in this PR:

  • add the image of the song in question
  • make the progress bar work
  • the next button appears when the player makes a guess or time's up

@jiabaow jiabaow self-assigned this Apr 18, 2022
}
}

private fun setNextButtonListener(ctx: Context, gameManager: GameManager) {
Copy link

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.


fun testClearResult() {
clearResult()
fun testGetAndCheckLyrics(ctx: Context, songName: String, artistName: String, speechInput: String, gameManager: GameManager) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method testGetAndCheckLyrics has 5 arguments (exceeds 4 allowed). Consider refactoring.

*/
private fun getAndCheckLyrics(songName: String, artistName: String, speechInput: String, gameManager: GameManager) {
private fun getAndCheckLyrics(ctx: Context, songName: String, artistName: String, speechInput: String, gameManager: GameManager) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method getAndCheckLyrics has 5 arguments (exceeds 4 allowed). Consider refactoring.

@jiabaow jiabaow marked this pull request as ready for review April 18, 2022 18:22
@jiabaow jiabaow linked an issue Apr 18, 2022 that may be closed by this pull request
Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work 👍 Potential quick thing to modify commented. What do you think ?

return !gameManager.checkGameStatus() || !gameManager.setNextSong()
}

fun superTestProgressBar(myBar: ProgressBar, progressTime: Int=0){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 106 is not fully tested. Idk if it can be easy to test it ?

if (hasWon) {
Toast.makeText(ctx, "$score Well Done!", Toast.LENGTH_SHORT).show()
} else {
Toast.makeText(ctx, "Sadly you're wrong", Toast.LENGTH_SHORT).show()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here we should use string.xml

*/
private fun hasWon(ctx: Context, score: Int, hasWon: Boolean) {
if (hasWon) {
Toast.makeText(ctx, "$score Well Done!", Toast.LENGTH_SHORT).show()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may use String.xml instead of hardcoding

} else {
findViewById<TextView>(R.id.lyricMatchResult).text =
"No lyrics found, try another song"
Toast.makeText(ctx, "No lyrics found, try the next song", Toast.LENGTH_SHORT).show()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May use string.xml file

startRound(ctx, gameManager)
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not urgent, but maybe refactor these two functions into one "set listeners of buttons" function.

Copy link
Collaborator

@zwierski zwierski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good work! Some minor refactoring to do but it's not urgent.

Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codeclimate
Copy link

codeclimate bot commented Apr 18, 2022

Code Climate has analyzed commit e98abab and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3
Duplication 2

The test coverage on the diff in this pull request is 96.4% (80% is the threshold).

This pull request will bring the total coverage in the repository to 88.6% (0.6% change).

View more on Code Climate.

@MaximeZmt MaximeZmt merged commit 4d3caf9 into main Apr 18, 2022
@MaximeZmt MaximeZmt deleted the jwen/enhance-lyric-game branch April 18, 2022 21:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redesign transition logic of lyric game (#156)
3 participants