feat: display computer moves #78
Conversation
Images were changed to buttons not accessible to user Also hotfix to determineResults function to correctly display winner.
adam-narozniak
force-pushed
the
feat/computer-moves
branch
from
ac84742
to
7935c7d
Compare
| when (hand) { | ||
| Hand.ROCK -> { | ||
| binding.rockRBOpponent.isChecked = true | ||
| } | ||
| Hand.PAPER -> { | ||
| binding.paperRBOpponent.isChecked = true | ||
| } | ||
| else -> { | ||
| binding.scissorsRB.isChecked = true | ||
| } |
There was a problem hiding this comment.
This should be exhaustive and not use else.
There was a problem hiding this comment.
Thanks, you're right. I've changed it now.
| val secondBestPoints = scores[1].points | ||
| if (bestPoints > secondBestPoints) { | ||
| val bestScoreList = mutableListOf<Round.Score>() | ||
| for (score in scores){ |
There was a problem hiding this comment.
Why not use a filter method to follow a more Kotlin-functional style?
There was a problem hiding this comment.
Thanks, I've changed it now.
There was a problem hiding this comment.
While I don't agree with some design choice and I think that providing more abstraction would be better, for example:
var gameService: OfflineGameService? = null
var currentUserResult: Hand.Result? = null
var computerPlayer: ComputerPlayer? = null
Why use hardcoded offline game service and not a general interface which could be both a offlinegameservice and an online one? Same discussion for computerplayer which might just be a Player (which internally can be extended with a computer player).
Other than that everything looks ok.
…bstracted in MatchViewModel
|
I checked out your branch and I found a small issue. When I finished the game and then showed the results, I think it could be better to go back to the main page when I click the "back" button. For now, when I click back, it shows the last round and I can even click to play the game, but what's strange is that the computer moves on the user side. |
|
@leonardopennino you're right. I've changed the gameService to the general one. As it's not critical I'll refactor the ComputerPlayer to inherit/implement the same things as User during the next sprint. |
|
@panadax Thanks, I'll change it to go back to the main page for now. Later I think there should be a new fragment and buttons "play again" and "end". |
After game over there is automatic transition to the main screen.
|
I think we should change the buttons. I'm personnally really not a big fan of the current ones. Maybe just using emojis could look better ? |
|
Sure we can do that but I don't think that it should be done in this PR. A new issue , sth like buttons customization is needed, and there the same way a user can change the mode it will be able to change the style of buttons. |
| delay(1000L) | ||
| resultNavigationCallback() | ||
| delay(1000L) |
There was a problem hiding this comment.
What are these delays for. You shouldn't add delays in the logic for no reason. Especially if it's to address a race condition.
There was a problem hiding this comment.
The delays are to let users notice the steps in the game. Otherwise, the transition would be too fast to spot.
For example, a change from the result(win/draw/loss) fragment back to the main screen has a break in between (for a user to notice the effect). In the same way, there's a break between an opponent's move and a transition to the result(win/draw/loss) page (so that they know what another player's choice was).
I hope it clarifies.
There was a problem hiding this comment.
Alright, make sure to comment to explain when you add delay, as it's very unusual so it has to be justified.
There was a problem hiding this comment.
Sure, I've added comments.
|
|
|
Yes, it is for a long time. Still, it's not this PR purpose for that. Also at the bottom, the buttons are cut. |
|
c.f #79 |
|
Sure, I can work on that but in the next sprint. And it's because we work according to the scrum workflow. So, in such a case like that, if there's a bug that is not critical (and the time spent fixing it might take long), you should put it on the backlog and then decide whether to work on that according to the priorities during the next sprint planning. Also, I've already worked the required by course time for this week's sprint. |
Images were changed to buttons not accessible to a user
Also, hotfix to determineResults function to correctly display result.