Margaux/name check #152

zwierski · 2022-04-06T09:42:25Z

This PR adds to the BuzzerSetupScreen a check that a name has been entered for every player: if one or more text fields are left empty, an alert appears upon clicking on proceed.

kamilababayeva

Nice work! I have tried the changes on the app, works like a charm. I have added few comments. I believe we should be able to test some pieces of the code.

kamilababayeva · 2022-04-07T17:11:05Z

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

@@ -24,15 +27,31 @@ class BuzzerScoreUpdater(ids: ArrayList<Int>, scores: Array<Int>) {
        return buzzerToScoreMap
    }

+    fun getWinnerId(): Int {
+        return winnerId


Codeclimate shows this line is not tested. This return should not be hard to test, right? Just calling the function and comparing the result to the actual value?

kamilababayeva · 2022-04-07T17:12:44Z

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,
+        val updatedScore = max(buzzerToScoreMap.getOrDefault(id, 0) + point, 0) // should never get to default


Did you put max considering that the score can be negative or what was the reason?

Oh, I added a functionality that gives the player a -1 penalty if they buzz and guess wrong (instead of keeping their score unchanged). If they guess wrong when their score is 0, the score should not become negative.

kamilababayeva · 2022-04-07T17:13:19Z

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

+     * @param potentialWinnerScore: that player's new score
+     */
+    private fun updateWinner(potentialWinnerId: Int, potentialWinnerScore: Int) {
+        if (winnerId<0 || potentialWinnerScore> buzzerToScoreMap[winnerId]!!) {


Suggested change

if (winnerId<0 || potentialWinnerScore> buzzerToScoreMap[winnerId]!!) {

if (winnerId < 0 || potentialWinnerScore > buzzerToScoreMap[winnerId]!!) {

Sorry hahah, this was hard to read, did not want to be mean

kamilababayeva · 2022-04-07T17:15:14Z

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

@@ -144,13 +145,15 @@ class BuzzerScreenActivity : AppCompatActivity() {
    private fun setAnswerButton(answer: LinearLayout, button: Button, updater: BuzzerScoreUpdater, map: Map<Int, Int>) {
        button.setOnClickListener {
            answer.visibility = android.view.View.INVISIBLE


Suggested change

answer.visibility = android.view.View.INVISIBLE

answer.visibility = View.INVISIBLE

kamilababayeva · 2022-04-07T17:23:00Z

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

@@ -144,13 +145,15 @@ class BuzzerScreenActivity : AppCompatActivity() {
    private fun setAnswerButton(answer: LinearLayout, button: Button, updater: BuzzerScoreUpdater, map: Map<Int, Int>) {


map is buzzersToRows in this function. BuzzersToRows is global in BuzzerScreenActivity class, so it is accessible in this function without passing it as a parameter. Do you think it is better if we remove map from here as a parameter and will access buzzerToRows inside the function?

kamilababayeva · 2022-04-07T17:24:46Z

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

+                    winnerId = updater.getWinnerId()
+                } else {updater.updateScoresArray(pressedBuzzer, -1)}
+                val view = map[pressedBuzzer]?.let { it1 -> findViewById<TextView>(it1) }
+                if (view != null && updater.getMap().keys.contains(pressedBuzzer)) {view.text=updater.getMap()[pressedBuzzer].toString()}
            }


Is this piece of code hard to test? You assign listener in onCreate, so if you do button.preform.click(), this code should be testable, no?

kamilababayeva · 2022-04-07T17:27:04Z

app/src/androidTest/java/ch/sdp/vibester/activity/BuzzerSetupActivityTest.kt

        onView(withId(R.id.nb_players_selected)).perform(click())
        intended(hasComponent(BuzzerScreenActivity::class.java.name))
        intended(hasExtra("Number of players", 4))
    }
-
+ */


what happened with this test? Other three works and this one no?

Yes, this one always fails for some reason while the other three always pass.

I have tried these tests myself, it worked in my android!

…meZmt/SDP_2022-Vibester into margaux/local-game-refactor

jiabaow

Good job!
I left some comments but they are mostly related to formatting issues

jiabaow · 2022-04-07T18:08:13Z

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

@@ -86,7 +91,11 @@ class BuzzerSetupActivity : AppCompatActivity(), AdapterView.OnItemSelectedListe
            arrayOf(R.id.namePlayer1, R.id.namePlayer2, R.id.namePlayer3, R.id.namePlayer4)
        var i = 0
        for (playerView in players) {
-            pNameArray[i] = findViewById<EditText>(editTextIdArray[i]).text.toString()
+            val name = findViewById<EditText>(editTextIdArray[i]).text.toString()
+            if (name.isNotEmpty()) { pNameArray[i] = name } else {


is there a particular reason why the if-else is formatted like this?

It was to remain under 25 lines of code for the function for the CodeClimate check 😅

ah okkkk, in this case, create a separate function then make a call to it would be a solution too

jiabaow · 2022-04-07T18:09:49Z

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

+        if (intent.hasExtra("Winner Name")) {
+            val winner = intent.getStringExtra("Winner Name")
+            /*
+            if (winner!=null) {


I would suggest either removing the commented code or adding a fixme explanation

jiabaow · 2022-04-07T18:09:57Z

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

-
+/*
+    fun hideWinnerPanel(view: View) {
+        findViewById<TextView>(R.id.winnerPanel).visibility=View.INVISIBLE


jiabaow · 2022-04-07T18:11:59Z

app/src/androidTest/java/ch/sdp/vibester/activity/BuzzerSetupActivityTest.kt

+     */
+    private fun enterNames(n: Int) {
+        var i = 0
+        while (i < n) { onView(withId(editTextIdArray[i])).perform(typeText(mockArray[i]), closeSoftKeyboard())


maybe move the onView... to a new line for better readability

jiabaow

LGTM

codeclimate · 2022-04-07T21:19:28Z

Code Climate has analyzed commit 70bb71d and detected 0 issues on this pull request.

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

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

View more on Code Climate.

zwierski added 7 commits April 4, 2022 20:35

prepare merge

29359fc

Merge branch 'main' into margaux/local-game-refactor

bacdec5

Merge branch 'main' into margaux/local-game-refactor

99ba4b5

Add missing name check

f7f3277

fix codeclimate issue

b71c1ec

retry ci w/o buggy test

c073e2e

add winner screen

fab5d54

zwierski linked an issue Apr 7, 2022 that may be closed by this pull request

Refactor local multiplayer game (default names, sanitization checks, connect winner name to ending screen). #140

Closed

zwierski added 3 commits April 7, 2022 15:15

remove winner screen until bugfix

f0654b0

Merge branch 'main' into margaux/local-game-refactor

583b88b

remove function for winner screen

0d5c713

zwierski marked this pull request as ready for review April 7, 2022 16:47

Merge branch 'main' into margaux/local-game-refactor

154c358

kamilababayeva assigned zwierski Apr 7, 2022

kamilababayeva approved these changes Apr 7, 2022

View reviewed changes

zwierski added 2 commits April 7, 2022 20:12

Merge branch 'main' into margaux/local-game-refactor

e186588

Merge branch 'margaux/local-game-refactor' of https://github.com/Maxi…

6e7d460

…meZmt/SDP_2022-Vibester into margaux/local-game-refactor

jiabaow suggested changes Apr 7, 2022

View reviewed changes

zwierski added 2 commits April 7, 2022 20:21

test winnerId in updater

ab19019

did requested changes

5091e51

jiabaow approved these changes Apr 7, 2022

View reviewed changes

fix build gradle junit version

70bb71d

zwierski merged commit 47f480e into main Apr 7, 2022

zwierski deleted the margaux/local-game-refactor branch April 7, 2022 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Margaux/name check #152

Margaux/name check #152

zwierski commented Apr 6, 2022

kamilababayeva left a comment

kamilababayeva Apr 7, 2022 •

edited

Loading

kamilababayeva Apr 7, 2022

zwierski Apr 7, 2022

kamilababayeva Apr 7, 2022

kamilababayeva Apr 7, 2022

kamilababayeva Apr 7, 2022

kamilababayeva Apr 7, 2022

kamilababayeva Apr 7, 2022

zwierski Apr 7, 2022

kamilababayeva Apr 7, 2022

jiabaow left a comment

jiabaow Apr 7, 2022

zwierski Apr 7, 2022

jiabaow Apr 7, 2022

jiabaow Apr 7, 2022

jiabaow Apr 7, 2022

jiabaow Apr 7, 2022

jiabaow left a comment

codeclimate bot commented Apr 7, 2022

	if (winnerId<0 \|\| potentialWinnerScore> buzzerToScoreMap[winnerId]!!) {
	if (winnerId < 0 \|\| potentialWinnerScore > buzzerToScoreMap[winnerId]!!) {

	answer.visibility = android.view.View.INVISIBLE
	answer.visibility = View.INVISIBLE

		@@ -144,13 +145,15 @@ class BuzzerScreenActivity : AppCompatActivity() {
		private fun setAnswerButton(answer: LinearLayout, button: Button, updater: BuzzerScoreUpdater, map: Map<Int, Int>) {

Margaux/name check #152

Margaux/name check #152

Conversation

zwierski commented Apr 6, 2022

kamilababayeva left a comment

Choose a reason for hiding this comment

kamilababayeva Apr 7, 2022 • edited Loading

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

jiabaow 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

jiabaow left a comment

Choose a reason for hiding this comment

codeclimate bot commented Apr 7, 2022

kamilababayeva Apr 7, 2022 •

edited

Loading