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 +6 -9
2 changed files with 6 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 28165d76d3 - Show all commits

9
src/Controller/GameController.php
View File

@ -112,11 +112,10 @@ class GameController implements ISecured
public function createNewChallenge(): IContent
{
// create Challenge
$challengeToken = rand();
while ($this->challengeRepository->getByToken($challengeToken)) {
// if a challenge with the same token already exists
$challengeToken = rand();
}
do {
balazs marked this conversation as resolved Outdated
bence commented 2021-05-27 22:56:17 +02:00
Outdated
Review
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
Outdated
Review
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
// initiliaze or if a challenge with the same token already exists
$challengeToken = mt_rand();
} while ($this->challengeRepository->getByToken($challengeToken));
bence commented 2021-05-27 23:06:31 +02:00
Outdated
Review
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
Outdated
Review
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
Outdated
Review
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
$challenge = new Challenge();
$challenge->setToken($challengeToken);

4
src/Repository/UserInChallengeRepository.php
View File

@ -57,11 +57,9 @@ class UserInChallengeRepository
}
// validate token string
foreach (str_split($token_str) as $char) {
if (!(('0' <= $char && $char <= '9') || ('a' <= $char && $char <= 'f'))) {
if (!ctype_xdigit($token_str)) {
balazs marked this conversation as resolved Outdated
bence commented 2021-05-27 23:09:13 +02:00
Outdated
Review
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
return null;
}
}
// convert token to int
$token = hexdec($token_str);