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

Kamila/refactor_game_choice_logic #146

Merged
merged 86 commits into from
Apr 4, 2022

Conversation

kamilababayeva
Copy link
Collaborator

@kamilababayeva kamilababayeva commented Apr 2, 2022

This PR rewrites the logic of settings for any game and connects Typing Game to the welcome screen. Now every game has the following common settings: choosing genre and choosing difficulty. After the settings are set up, the individual game activities will start.

Major changes:

  • WelcomeActivity is now the main launcher activity
  • Previous MainActivity was deleted together with tests
  • Common setting (genre, difficulty) for all the games
  • GenreTemporaryActivity is merged with GameSetupActivity
  • Appropriate naming for Buzzer game
  • Set style for choosing genre

@kamilababayeva kamilababayeva marked this pull request as ready for review April 3, 2022 19:05
@@ -153,8 +153,8 @@ class LyricsBelongGameActivityTest {
scn.onActivity { activity ->
activity.testGetAndCheckLyrics("the best song in the world", "Mr.Mystery", "", gameManager)
}
Thread.sleep(sleepTime)
onView(withId(R.id.lyricMatchResult)).check(matches(withText("No lyrics found, try another song")))
// Thread.sleep(10000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe delete these if they're unecessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sometime tests are not working because of the timing issue. I dont want to remove them so we remember to modify them

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, you can add a @fixme. I will try to mock the API call once the mock library is set up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

* @param
* n: number of players selected in the spinner
* id: the id of the field to update
* Fetch data from Lastfm and set song list in a GameManager
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe add an explanation what the parameter uri is in the javadoc

Copy link
Collaborator

@laurislopata laurislopata left a comment

Choose a reason for hiding this comment

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

Vey good job, just some very minor details but I'd even without them LGTM

switchToGame(TypingGameActivity())
}
else if(this.game == "local_lyrics"){
switchToGame(LyricsBelongGameActivity())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it normal that the lyrics game is not tested?

Copy link
Collaborator Author

@kamilababayeva kamilababayeva Apr 4, 2022

Choose a reason for hiding this comment

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

I have the tests for it:
fun localTypingOnClick(){
onView(withId(R.id.local_typing_game_button)).perform(scrollTo(), click())
onView(withId(R.id.btsButton)).perform(click())
onView(withId(R.id.difficulty_spinner)).perform(click())
intended(hasComponent(TypingGameActivity::class.java.name))
intended(hasExtra("Difficulty", "Hard"))
}

I have tested for random difficulty and genre. Do you think it is enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, this one I am not... I will test it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@@ -153,8 +153,8 @@ class LyricsBelongGameActivityTest {
scn.onActivity { activity ->
activity.testGetAndCheckLyrics("the best song in the world", "Mr.Mystery", "", gameManager)
}
Thread.sleep(sleepTime)
onView(withId(R.id.lyricMatchResult)).check(matches(withText("No lyrics found, try another song")))
// Thread.sleep(10000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, you can add a @fixme. I will try to mock the API call once the mock library is set up.


/**
* Converts the spinner text for the number of players into an Int
* @param
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @param
* @param text: the string to be converted

"Three" -> return 3
"Four" -> return 4
}
return 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to test the return value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not my code, but I will try!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!


val statNames: ArrayList<String> = mockArray

val statVal: ArrayList<String> = mockArray
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good change, makes more sense to have all the tests values in variables.

val updater = BuzzerScoreUpdater(buzIds, allPoints)

val buzToScoreMap = fetchBuzToScoreRowMap()
val updater = BuzzerScoreUpdater(buzIds, arrayListOf(*allPoints))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe change the declaration of allPoints to an ArrayList if possible, it might be simpler than declaring an array and then casting it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

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.

Good job on refactoring so much code! Everything looks good to me, there is one change I commented on (the array declaration) but I can take care of it since it's part of the code I've written.

@codeclimate
Copy link

codeclimate bot commented Apr 4, 2022

Code Climate has analyzed commit 103db5d and detected 0 issues on this pull request.

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

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

View more on Code Climate.

@kamilababayeva kamilababayeva merged commit 37ce25a into main Apr 4, 2022
@kamilababayeva kamilababayeva deleted the kamila/refactor_game_choice_logic branch April 4, 2022 17:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants