Margaux/buzzer score updater #128

zwierski · 2022-03-31T14:11:18Z

This PR contains the new class BuzzerScoreUpdater, which keeps track of a local game's scores and updates them when a user correctly guesses a song.

codeclimate · 2022-03-31T14:27:28Z

app/src/main/java/ch/sdp/vibester/activity/GamescreenActivity.kt

+        }
+        setAnswerButton(answer, findViewById(R.id.buttonCorrect), updater, buzToScoreMap)
+        setAnswerButton(answer, findViewById(R.id.buttonWrong), updater, buzToScoreMap)
+    }


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2022-03-31T14:27:28Z

app/src/main/java/ch/sdp/vibester/activity/GamescreenActivity.kt

+        setAnswerButton(answer, findViewById(R.id.buttonCorrect), updater, buzToScoreMap)
+        setAnswerButton(answer, findViewById(R.id.buttonWrong), updater, buzToScoreMap)
+    }
+


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2022-03-31T15:06:52Z

app/src/main/java/ch/sdp/vibester/activity/GamescreenActivity.kt

 import ch.sdp.vibester.R


 class GamescreenActivity: AppCompatActivity() {

+    private val MAX_N_PLAYERS = 4
+    private val NO_BUZZER_PRESSED = -1
+    private val rowsIdArray = arrayOf(R.id.row_0, R.id.row_1, R.id.row_2, R.id.row_3)


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2022-03-31T15:06:52Z

app/src/main/java/ch/sdp/vibester/activity/GamescreenActivity.kt

+    private val MAX_N_PLAYERS = 4
+    private val NO_BUZZER_PRESSED = -1
+    private val rowsIdArray = arrayOf(R.id.row_0, R.id.row_1, R.id.row_2, R.id.row_3)
+    private val buzIds = arrayOf(R.id.buzzer_0, R.id.buzzer_1, R.id.buzzer_2, R.id.buzzer_3)


Similar blocks of code found in 2 locations. Consider refactoring.

Would not really make sense to refactor, this is just declaration and initialisation of 2 arrays with values from the xml.

Agreed. Worst case, if these buzzers and rows are the only children of the layout, can iterate over them and use getChild to set their values. Leaving the comment here in case we need to do so in the future.

Tsathogguaa · 2022-03-31T15:30:53Z

app/src/androidTest/java/ch/sdp/vibester/BuzzerScoreUpdaterTest.kt

@@ -6,25 +6,43 @@ import org.junit.Test

 public class BuzzerScoreUpdaterTest {

-    // FIXME: Uncomment this when the infinite loop issue in the class is fixed


Is it fixed? If so, maybe it's time to remove this :)

Tsathogguaa

Looks good!

kamilababayeva

Super happy that now the scores are stored! Nice work!

There is a problem with the use of assert in the testing, please take a look. Thanks!

kamilababayeva · 2022-03-31T19:33:18Z

app/src/androidTest/java/ch/sdp/vibester/BuzzerScoreUpdaterTest.kt

+        }
+        for (value in scoreArray) {
+            assert(testUpdater.getMap().values.contains(value))
+        }


I was searching and I believe it is not correct to use java's assert in the test. Because in this case the java's exception will appear, not junit's exception. I would change assert on junit's assertTrue or assertEquals.

Also here ExpectedMap is not used, so it can be deleted

Good point, I'll change this!

kamilababayeva · 2022-03-31T19:34:06Z

app/src/androidTest/java/ch/sdp/vibester/BuzzerScoreUpdaterTest.kt

+        val testUpdater = BuzzerScoreUpdater(idArray, scoreArray)
+        testUpdater.updateScoresArray(-1)
+        for (id in idArray) {
+            assert(testUpdater.getMap()[id]==0)


Suggested change

assert(testUpdater.getMap()[id]==0)

assertTrue(testUpdater.getMap()[id]==0)

kamilababayeva · 2022-03-31T19:34:15Z

app/src/androidTest/java/ch/sdp/vibester/BuzzerScoreUpdaterTest.kt

+        val testUpdater = BuzzerScoreUpdater(idArray, scoreArray)
+        for (id in idArray) {
+            testUpdater.updateScoresArray(id)
+            assert(testUpdater.getMap()[id]==1)


Suggested change

assert(testUpdater.getMap()[id]==1)

assertTrue(testUpdater.getMap()[id]==1)

kamilababayeva · 2022-03-31T19:39:17Z

app/src/main/java/ch/sdp/vibester/BuzzerScoreUpdater.kt

+    private fun initMap(ids: Array<Int>, scores: Array<Int>): LinkedHashMap<Int, Int> {
+        var theMap = LinkedHashMap<Int, Int>()
+        var i = 0
+        while (i < min(ids.size, scores.size)) {


Why do you take min here? Should not both arrays be the same size?

When I tried to perform a same size check and then use one of the sizes in the while loop, the app kept crashing, even though I only have cases where the scores are initialized according to the number of players (so should always be the same size). I used the min function as a quick fix for this, and I'll try to implement a proper check when refactoring.

kamilababayeva · 2022-03-31T19:40:45Z

app/src/main/java/ch/sdp/vibester/BuzzerScoreUpdater.kt

+        if (!buzzerToScoreMap.keys.contains(id)) {
+            return
+        }
+        val updatedScore = buzzerToScoreMap.getOrDefault(id, 0) + 1 // should never get to default,


If the buzzer is initiated, there is no need for default, right?

I tried using the "get" function: however, it returns an Int? which is not castable to Int, even with a null check.

also here you can update the score in buzzer directly, like that:
buzzerToScoreMap[id]?.plus(1)

kamilababayeva · 2022-03-31T19:42:08Z

app/src/main/java/ch/sdp/vibester/BuzzerScoreUpdater.kt

+     * checks first if the given id
+     */
+    fun updateScoresArray(id: Int) {
+        if (!buzzerToScoreMap.keys.contains(id)) {


Is this check is needed as well? The array will be initialized with exactly entered number of ids, so there could not be a missing id in general... or am I missing something?

This check ensures that no update will be attempted if the parameter is equal to NO_BUZZER_PRESSED (which should not happen but I still added a check just in case). I'll update the doc to make this clearer.

codeclimate · 2022-03-31T20:44:56Z

Code Climate has analyzed commit 4109b04 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2

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

This pull request will bring the total coverage in the repository to 90.6%.

View more on Code Climate.

zwierski added 2 commits March 31, 2022 15:59

Buzzer Score Updater connected V1

5acc8e7

Merge branch 'main' into margaux/buzzer-score-updater

589dc80

codeclimate bot reviewed Mar 31, 2022

View reviewed changes

Add tests for updater and refactor activity

bb279c5

codeclimate bot reviewed Mar 31, 2022

View reviewed changes

zwierski marked this pull request as ready for review March 31, 2022 15:13

Tsathogguaa reviewed Mar 31, 2022

View reviewed changes

Tsathogguaa approved these changes Mar 31, 2022

View reviewed changes

zwierski linked an issue Mar 31, 2022 that may be closed by this pull request

Fix BuzzerScoreUpdater (#57) #108

Closed

zwierski self-assigned this Mar 31, 2022

kamilababayeva suggested changes Mar 31, 2022

View reviewed changes

zwierski added 2 commits March 31, 2022 22:16

Fix asserts and doc

5e7494e

Fix conflicts from main

4109b04

kamilababayeva self-requested a review March 31, 2022 20:32

kamilababayeva approved these changes Mar 31, 2022

View reviewed changes

zwierski merged commit 597e67a into main Mar 31, 2022

zwierski deleted the margaux/buzzer-score-updater branch March 31, 2022 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Margaux/buzzer score updater #128

Margaux/buzzer score updater #128

zwierski commented Mar 31, 2022 •

edited

Loading

codeclimate bot Mar 31, 2022

codeclimate bot Mar 31, 2022

codeclimate bot Mar 31, 2022

codeclimate bot Mar 31, 2022

zwierski Mar 31, 2022

Tsathogguaa Mar 31, 2022

Tsathogguaa Mar 31, 2022

Tsathogguaa left a comment

kamilababayeva left a comment

kamilababayeva Mar 31, 2022

kamilababayeva Mar 31, 2022

zwierski Mar 31, 2022

kamilababayeva Mar 31, 2022

kamilababayeva Mar 31, 2022

kamilababayeva Mar 31, 2022

zwierski Mar 31, 2022

kamilababayeva Mar 31, 2022

zwierski Mar 31, 2022

kamilababayeva Mar 31, 2022

kamilababayeva Mar 31, 2022 •

edited

Loading

zwierski Mar 31, 2022

codeclimate bot commented Mar 31, 2022

		@@ -6,25 +6,43 @@ import org.junit.Test

		public class BuzzerScoreUpdaterTest {

		// FIXME: Uncomment this when the infinite loop issue in the class is fixed

	assert(testUpdater.getMap()[id]==0)
	assertTrue(testUpdater.getMap()[id]==0)

	assert(testUpdater.getMap()[id]==1)
	assertTrue(testUpdater.getMap()[id]==1)

Margaux/buzzer score updater #128

Margaux/buzzer score updater #128

Conversation

zwierski commented Mar 31, 2022 • edited Loading

codeclimate bot Mar 31, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tsathogguaa left a comment

Choose a reason for hiding this comment

kamilababayeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamilababayeva Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 31, 2022

zwierski commented Mar 31, 2022 •

edited

Loading

kamilababayeva Mar 31, 2022 •

edited

Loading