-
Notifications
You must be signed in to change notification settings - Fork 0
Margaux/buzzer timer and music backend #206
Conversation
if (players != null) { | ||
buildScores(players, allPoints) | ||
buildBuzzers(players, answer) | ||
/** |
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.
super.onDestroy() | ||
} | ||
*/ | ||
/** |
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.
fun checkAnswer(ctx: Context, chosenSong: Song?, gameManager: BuzzerGameManager) { | ||
val playedSong = gameManager.getCurrentSong() | ||
|
||
if (chosenSong != null && chosenSong.getTrackName() == playedSong.getTrackName() && chosenSong.getArtistName() == playedSong.getArtistName()) { |
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.
Identical 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.
Maybe we should create some util file and store some functions that all games use to avoid this duplication of code?
Code Climate has analyzed commit 2c739b4 and detected 6 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 32.2% (80% is the threshold). This pull request will bring the total coverage in the repository to 84.2% (-2.2% change). View more on Code Climate. |
override fun run() { | ||
if (myBar.progress > 0) { | ||
decreaseBarTimer(myBar) | ||
handler.postDelayed(this, 999) //just a bit shorter than a second for safety |
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 create a variable instead of hardcoding it?
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.
Added some comments, coverage is an issue but as you said you'll focus on that next week
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.
Since you said you will work on fixing the codeclimeate issues next week. I think we can merge this. I'll approve it when te test pass
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.
thank for your update! it is great! I have left some comments to resolve.
Also, a bug for us:
when the game reaches the maximum of fetched songs (100 in our case), it is stuck repeating the same song. If we have time in the end of the semester, we can solve it.
val getIntent = intent.extras | ||
val nPlayers = getIntent?.getInt("Number of players") | ||
if (getIntent != null) { |
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.
rename getIntent to intentExtras or something like that
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 am not sure whether it is okay to keep all code under if statement. In other activities we just check if the extra exists.
val answer = findViewById<LinearLayout>(R.id.answer) | ||
answer.visibility=View.INVISIBLE | ||
//val answerText = findViewById<TextView>(R.id.answerText) | ||
//answerText.text = "The song was Demo by The Placeholders" |
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.
remove if not used
|
||
// fetch song and initialise answerText. We'll see later for the image | ||
val title = gameManager.getCurrentSong().getTrackName() | ||
findViewById<TextView>(R.id.answerText).text=title |
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.
findViewById<TextView>(R.id.answerText).text=title | |
findViewById<TextView>(R.id.answerText).text = title | |
findViewById<LinearLayout>(R.id.answer).visibility=View.INVISIBLE | ||
|
||
// fetch song and initialise answerText. We'll see later for the image | ||
val title = gameManager.getCurrentSong().getTrackName() |
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 suggest to set both title and artistname in the answer
} | ||
|
||
|
||
/** | ||
* Fires an intent from the Gamescreen to the Ending Screen | ||
*/ | ||
fun switchToEnding(view: View) { | ||
if (gameManager.playingMediaPlayer()) { |
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 had a bug that the game suddenly switched to end without stopping the media player. I am not sure how to reproduce the error, i have been playing a lot.
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 it happened when the super.enRound is used and it goes to super.switvhToEnding
import ch.sdp.vibester.api.AudioPlayer | ||
import java.util.concurrent.CompletableFuture | ||
|
||
class BuzzerGameManager: GameManager() { |
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 is vrey similar to typinggameManager, we should create something common
if (players != null) { | ||
buildScores(players, allPoints) | ||
buildBuzzers(players, answer) | ||
if (players != null) { |
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.
In general, you always have default values for players, so mayber ther is no need to check for that
android:gravity="center" | ||
android:orientation="vertical" | ||
app:layout_constraintBottom_toBottomOf="parent" | ||
app:layout_constraintEnd_toEndOf="parent" | ||
app:layout_constraintHorizontal_bias="1.0" | ||
app:layout_constraintStart_toStartOf="parent" | ||
app:layout_constraintTop_toBottomOf="@+id/scoresTable"> | ||
app:layout_constraintTop_toBottomOf="@+id/go_to_end"> | ||
|
||
|
||
</LinearLayout> | ||
|
||
<TableLayout | ||
android:id="@+id/scoresTable" |
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 should make the font bigger for name and score
@@ -94,7 +140,7 @@ class BuzzerScreenActivityTest { | |||
onView(withId(R.id.answer)).check(matches(withEffectiveVisibility(Visibility.INVISIBLE))) | |||
} | |||
} | |||
|
|||
/* | |||
/* | |||
* Currently testing with the *static* values. Change to *dynamic* once the game is correctly |
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 fails because gameManager was not initialized:
Caused by: kotlin.UninitializedPropertyAccessException: lateinit property gameManager has not been initialized
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 comments, nice work in overall 👍
|
||
class BuzzerScreenActivity : GameActivity() { |
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.
class BuzzerScreenActivity : GameActivity() { | |
/** | |
* docs ? | |
*/ | |
class BuzzerScreenActivity : GameActivity() { | |
val getIntent = intent.extras | ||
val nPlayers = getIntent?.getInt("Number of players") | ||
if (getIntent != null) { | ||
|
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.
//val answerText = findViewById<TextView>(R.id.answerText) | ||
//answerText.text = "The song was Demo by The Placeholders" |
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.
Are these comments useful ?
setAnswerButton(ctx, answer, findViewById(R.id.buttonCorrect), updater, buzzersToRows) | ||
setAnswerButton(ctx, answer, findViewById(R.id.buttonWrong), updater, buzzersToRows) | ||
|
||
// null pointer? |
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 ? Maybe either make it clearer if we keep it or remove it
|
||
// fetch song and initialise answerText. We'll see later for the image | ||
val title = gameManager.getCurrentSong().getTrackName() | ||
findViewById<TextView>(R.id.answerText).text=title |
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.
findViewById<TextView>(R.id.answerText).text=title | |
findViewById<TextView>(R.id.answerText).text = title | |
*/ | ||
private fun startRound(ctx: Context, gameManager: BuzzerGameManager) { | ||
gameIsOn = true | ||
findViewById<LinearLayout>(R.id.answer).visibility=View.INVISIBLE |
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.
findViewById<LinearLayout>(R.id.answer).visibility=View.INVISIBLE | |
findViewById<LinearLayout>(R.id.answer).visibility = View.INVISIBLE | |
//TODO: replace with score handler stuff here | ||
//hasWon(ctx, gameManager.getScore(), true, playedSong) | ||
} else { | ||
gameManager.addWrongSong() | ||
//hasWon(ctx, gameManager.getScore(), false, playedSong) |
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.
Are the comments useful to keep ?
//findViewById<EditText>(R.id.yourGuessET).isEnabled = false | ||
//checkRunnable() |
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.
Useful to keep ?
@@ -143,7 +144,7 @@ class GameSetupActivity : AppCompatActivity(), AdapterView.OnItemSelectedListene | |||
*/ | |||
fun chooseGame(view: View){ | |||
when (view.id) { | |||
R.id.local_buzzer_game_button -> {game = "local_buzzer"; gameManager = GameManager()} | |||
R.id.local_buzzer_game_button -> {game = "local_buzzer"; gameManager = BuzzerGameManager() } |
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.
R.id.local_buzzer_game_button -> {game = "local_buzzer"; gameManager = BuzzerGameManager() } | |
R.id.local_buzzer_game_button -> {game = "local_buzzer"; gameManager = BuzzerGameManager()} |
} | ||
} | ||
|
||
/** |
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.
Identical blocks of code found in 2 locations. Consider refactoring.
@@ -140,7 +255,7 @@ class BuzzerScreenActivity : AppCompatActivity() { | |||
* @param updater: the updater for the scores | |||
* @param map: a map from the buzzers' IDs to the IDs of each score's position in the score table layout | |||
*/ | |||
private fun setAnswerButton(answer: LinearLayout, button: Button, updater: BuzzerScoreUpdater, map: Map<Int, Int>) { | |||
private fun setAnswerButton(ctx: Context, answer: LinearLayout, button: Button, updater: BuzzerScoreUpdater, map: Map<Int, Int>) { |
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.
Method setAnswerButton
has 5 arguments (exceeds 4 allowed). 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.
As it is late, I approved !!exceptionally!! whereas you didn't address all the issues. Try to have a pr a bit earlier next week. 😅
Please address the issues in comment this weekend !!
This PR contains the implementation of a game Manager for the buzzer game, as well as an added progress bar in the buzzer screen's layout. The game can now play songs and display their titles (not yet their album artwork) in the answer card, and switches song after each answer.