esoko/mapguesser
esoko/mapguesser
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 18 Wiki Activity

feature/MAPG-235-basic-challenge-mode #48

Merged
balazs merged 43 commits from feature/MAPG-235-basic-challenge-mode into develop 2021-05-28 20:41:09 +02:00
Conversation 0 Commits 43 Files Changed 28 +1841 -142
balazs commented 2021-05-22 21:21:19 +02:00
Member
Copy Link
No description provided.
challenge.png
89 KiB
challenge.png
balazs added 38 commits 2021-05-22 21:21:19 +02:00
balazs requested review from bence 2021-05-22 21:21:31 +02:00
balazs reviewed 2021-05-23 12:28:47 +02:00
public/static/js/game.js Outdated
@ -296,0 +382,4 @@
var roundContainer = document.getElementById('roundContainer');
var header = roundContainer.parentNode;
header.insertBefore(restrictions, roundContainer);
balazs commented 2021-05-23 12:28:47 +02:00
Author
Member
Copy Link

I had to create span from javascript, because I didn't know, how else to control the display css attribute from both css (depending on screen size) and javascript (depending on the game type). Javascript overrides the css attribute even for narrow screens. Is there a more elegant solution?

I had to create span from javascript, because I didn't know, how else to control the display css attribute from both css (depending on screen size) and javascript (depending on the game type). Javascript overrides the css attribute even for narrow screens. Is there a more elegant solution?
bence commented 2021-05-27 23:39:14 +02:00
Owner
Copy Link

It could be solved with a hidden element, for example with class hidden which can be removed by JavaScript when it should be shown (it still can have class hideOnNarrowScreen). But I think it is also a good solution.

It could be solved with a hidden element, for example with class `hidden` which can be removed by JavaScript when it should be shown (it still can have class `hideOnNarrowScreen`). But I think it is also a good solution.
balazs marked this conversation as resolved
bence started working 2021-05-23 21:25:17 +02:00
bence reviewed 2021-05-25 18:13:16 +02:00
views/maps.php Outdated
@ -32,0 +67,4 @@
</div>
<div>
<input type="checkbox" id="noZoom" name="noZoom" value="noZoom" />
<label for="noMove">No zoom allowed</label>
bence commented 2021-05-25 18:13:16 +02:00
Owner
Copy Link
<label for="noZoom">
```html <label for="noZoom"> ```
balazs marked this conversation as resolved
bence reviewed 2021-05-25 18:13:59 +02:00
views/maps.php
@ -13,1 +13,4 @@
<button id="multiButton" class="fullWidth green" data-map-id="">Multiplayer (beta)</button>
<?php if ($isLoggedIn): ?>
<p class="bold center marginTop marginBottom">OR</p>
<button id="challengeButton" class="fullWidth yellow" data-map-id="" data-timer="">Challenge (gamma)</button>
bence commented 2021-05-25 18:13:59 +02:00
Owner
Copy Link

Gamma :D

Gamma :D
balazs added 2 commits 2021-05-26 08:16:42 +02:00
bence reviewed 2021-05-27 22:56:17 +02:00
src/Controller/GameController.php Outdated
@ -88,0 +112,4 @@
public function createNewChallenge(): IContent
{
// create Challenge
$challengeToken = rand();
bence commented 2021-05-27 22:56:17 +02:00
Owner
Copy Link

Maybe a do..while would be better here because the token calculation should not be repeated.

        do {
            // if a challenge with the same token already exists
            $challengeToken = rand();
        } while ($this->challengeRepository->getByToken($challengeToken));
Maybe a do..while would be better here because the token calculation should not be repeated. ```php do { // if a challenge with the same token already exists $challengeToken = rand(); } while ($this->challengeRepository->getByToken($challengeToken)); ```
balazs commented 2021-05-27 23:34:00 +02:00
Author
Member
Copy Link

My eye is twitching when variables are used outside of the scope of {}, but it's different in PHP, so it can be refactored.

My eye is twitching when variables are used outside of the scope of {}, but it's different in PHP, so it can be refactored.
😆 1
balazs marked this conversation as resolved
bence reviewed 2021-05-27 23:06:31 +02:00
src/Controller/GameController.php Outdated
@ -88,0 +115,4 @@
$challengeToken = rand();
while ($this->challengeRepository->getByToken($challengeToken)) {
// if a challenge with the same token already exists
$challengeToken = rand();
bence commented 2021-05-27 23:06:31 +02:00
Owner
Copy Link

I think rand() should be called with explicit arguments, otherwise a number is returned between 0 and getrandmax() and getrandmax() is platform-dependent.

On the other hand maybe the token could be generated as the room ID for multiplayer. Then a fixed length string would be generated - I used bin2hex(random_bytes(3)) for room ID. I guess it was intentional to use integer indexes in the DB, it could be be more efficient but I already used string indexes for other purpose.

I think rand() should be called with explicit arguments, otherwise a number is returned between 0 and getrandmax() and getrandmax() is platform-dependent. On the other hand maybe the token could be generated as the room ID for multiplayer. Then a fixed length string would be generated - I used bin2hex(random_bytes(3)) for room ID. I guess it was intentional to use integer indexes in the DB, it could be be more efficient but I already used string indexes for other purpose.
balazs commented 2021-05-27 23:36:23 +02:00
Author
Member
Copy Link

Good point. However I don't think it would cause any problems. I thought it would be better to use 4 byte length integers instead of 3 bytes for more possible ids, because challenges are normally not getting deleted.

Good point. However I don't think it would cause any problems. I thought it would be better to use 4 byte length integers instead of 3 bytes for more possible ids, because challenges are normally not getting deleted.
balazs commented 2021-05-28 07:42:05 +02:00
Author
Member
Copy Link

I've read your comment again, and I see that bin2hex returns a string. Yes I thought it would perform better just to use an integer instead.
I can also see now, that on Windows the getrandmax() is only 32767, which is far too small. I am going to replace it with mt_rand() if that's fine for you.

I've read your comment again, and I see that bin2hex returns a string. Yes I thought it would perform better just to use an integer instead. I can also see now, that on Windows the getrandmax() is only 32767, which is far too small. I am going to replace it with mt_rand() if that's fine for you.
👍 1
bence reviewed 2021-05-27 23:09:13 +02:00
src/Repository/UserInChallengeRepository.php Outdated
@ -0,0 +57,4 @@
}
// validate token string
foreach (str_split($token_str) as $char) {
bence commented 2021-05-27 23:09:13 +02:00
Owner
Copy Link

PHP's builtin ctype_xdigit could be used for that.

PHP's builtin [ctype_xdigit](https://www.php.net/manual/en/function.ctype-xdigit.php) could be used for that.
👍 1
balazs marked this conversation as resolved
bence reviewed 2021-05-27 23:16:29 +02:00
public/static/js/game.js
@ -1,10 +1,13 @@
'use strict';
bence commented 2021-05-27 23:16:29 +02:00
Owner
Copy Link

This file should be refactored sometime because it is very spaghetti :D

This file should be refactored sometime because it is very spaghetti :D
balazs commented 2021-05-27 23:37:40 +02:00
Author
Member
Copy Link

I know, but I already put a lot of changes into this PR. I think we should refactor it in a separate story.

I know, but I already put a lot of changes into this PR. I think we should refactor it in a separate story.
bence commented 2021-05-27 23:40:16 +02:00
Owner
Copy Link

Yes, I meant later, it will be a bigger task.

Yes, I meant later, it will be a bigger task.
balazs marked this conversation as resolved
bence reviewed 2021-05-27 23:20:45 +02:00
public/static/js/game.js
@ -535,0 +713,4 @@
if (Game.timeoutEnd) {
var timeLeft = Math.ceil((Game.timeoutEnd - new Date()) / 1000);
data.append('timeLeft', timeLeft);
bence commented 2021-05-27 23:20:45 +02:00
Owner
Copy Link

So actually users can cheat with this if they send a bigger number back :D

So actually users can cheat with this if they send a bigger number back :D
balazs commented 2021-05-27 23:41:31 +02:00
Author
Member
Copy Link

Users can even cheat, if they just reload the page, because that resets the timer, or just by pausing the script. That's a bigger issue. But because PHP doesn't support websockets and I didn't want to create a request every second to the server to update the time, it's still an open topic. That's why I think, we should discuss an appropiate solution in a future story for it. I think most people won't figure this out anyway.

Users can even cheat, if they just reload the page, because that resets the timer, or just by pausing the script. That's a bigger issue. But because PHP doesn't support websockets and I didn't want to create a request every second to the server to update the time, it's still an open topic. That's why I think, we should discuss an appropiate solution in a future story for it. I think most people won't figure this out anyway.
👍 1
bence commented 2021-05-28 20:24:14 +02:00
Owner
Copy Link

I think Websocket is not really necessary for that, it could be implemented without "active" connection. For example we could store a timestamp for each guess and calculate the difference between the current timestamp and the timestamp of the last guess (or challange start). So when user refreshes the page the timer is not restarted (and guesses can be rejected if time is up). But it could be done later.

I think Websocket is not really necessary for that, it could be implemented without "active" connection. For example we could store a timestamp for each guess and calculate the difference between the current timestamp and the timestamp of the last guess (or challange start). So when user refreshes the page the timer is not restarted (and guesses can be rejected if time is up). But it could be done later.
balazs commented 2021-05-28 20:35:30 +02:00
Author
Member
Copy Link

That's a good idea. I'll write it into the ticket.

That's a good idea. I'll write it into the ticket.
bence reviewed 2021-05-27 23:22:04 +02:00
public/static/js/game.js
@ -732,0 +963,4 @@
var summaryInfo = document.getElementById('summaryInfo');
if (highscores.length > 2) {
var table = document.getElementById('highscoresTable');
bence commented 2021-05-27 23:22:04 +02:00
Owner
Copy Link

This highscore table could be implemented for multiplayer in the future.

This highscore table could be implemented for multiplayer in the future.
balazs commented 2021-05-27 23:45:01 +02:00
Author
Member
Copy Link

Yes, that was my intention as well. I just couldn't find the time for implementing and properly testing it, and it's also isn't really part of this story.

Yes, that was my intention as well. I just couldn't find the time for implementing and properly testing it, and it's also isn't really part of this story.
bence reviewed 2021-05-27 23:34:32 +02:00
public/static/css/maps.css Outdated
@ -75,6 +75,31 @@ div.mapItem>div.buttonContainer {
grid-auto-flow: column;
}
#restrictions {
bence commented 2021-05-27 23:34:32 +02:00
Owner
Copy Link

I think some of the rules should be handled on higher level, for example font-family in mapguesser.css line 34,
margin-top and margin-bottom could be handled by classes marginTop and marginBottom, etc.

But it is fine for now because some rules would require refactoring (for example for inputs).

I think some of the rules should be handled on higher level, for example `font-family` in mapguesser.css line 34, `margin-top` and `margin-bottom` could be handled by classes `marginTop` and `marginBottom`, etc. But it is fine for now because some rules would require refactoring (for example for inputs).
balazs marked this conversation as resolved
balazs added 1 commit 2021-05-28 08:07:18 +02:00
balazs added 1 commit 2021-05-28 08:19:51 +02:00
balazs added 1 commit 2021-05-28 08:37:00 +02:00
bence approved these changes 2021-05-28 20:24:35 +02:00
bence stopped working 2021-05-28 20:27:48 +02:00
119h 2min 31s
balazs merged commit 784037de6f into develop 2021-05-28 20:41:09 +02:00
bence deleted branch feature/MAPG-235-basic-challenge-mode 2021-05-29 00:16:23 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
bence
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Total Time Spent: 4 days 23 hours
bence
4 days 23 hours
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: esoko/mapguesser#48
No description provided.
Delete Branch "feature/MAPG-235-basic-challenge-mode"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?