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

feature/avoid-repeating-places-in-game #38

Merged
balazs merged 14 commits from feature/avoid-repeating-places-in-game into develop 2021-05-09 10:58:54 +02:00
Conversation 1 Commits 14 Files Changed 10 +23 -19
2 changed files with 23 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 3f8311d708 - Show all commits

9
src/Database/Query/Select.php
View File

@ -328,14 +328,13 @@ class Select
private function generateJoins(): array private function generateJoins(): array
{ {
$joins = $this->joins;
$joinQueries = []; $joinQueries = [];
$params = []; $params = [];
foreach($joins as $value) { foreach($this->joins as $join) {
list($joinQueryFragment, $paramsFragment) = $this->generateTable($value[1], true); list($joinQueryFragment, $paramsFragment) = $this->generateTable($join[1], true);
bence commented 2021-05-08 21:39:03 +02:00
Outdated
Review
Copy Link

If it is refactored anyway I would directly use $this->joins and local variable $joins (line 331) becomes unnecessary.

And calling the loop variable $join instead of $value would be nicer.

foreach($this->joins as $join) {
If it is refactored anyway I would directly use `$this->joins` and local variable `$joins` (line 331) becomes unnecessary. And calling the loop variable `$join` instead of `$value` would be nicer. ```php foreach($this->joins as $join) { ```
array_push($joinQueries, $value[0] . ' JOIN ' . $joinQueryFragment . ' ON ' . $this->generateColumn($value[2]) . ' ' . $value[3] . ' ' . $this->generateColumn($value[4])); $joinQueries[] = $join[0] . ' JOIN ' . $joinQueryFragment . ' ON ' . $this->generateColumn($join[2]) . ' ' . $join[3] . ' ' . $this->generateColumn($join[4]);
$params = array_merge($params, $paramsFragment); $params = array_merge($params, $paramsFragment);
bence commented 2021-05-08 21:35:04 +02:00
Review
Copy Link

Better to use $array[] = to add a new element to an array because it is more efficient according the the PHP docs:

$joinQueries[] = $value[0] . ' JOIN ' . $joinQueryFragment . ' ON ' . $this->generateColumn($value[2]) . ' ' . $value[3] . ' ' . $this->generateColumn($value[4]));
Better to use `$array[] =` to add a new element to an array because it is more efficient according the the [PHP docs](https://www.php.net/manual/en/function.array-push.php): ```php $joinQueries[] = $value[0] . ' JOIN ' . $joinQueryFragment . ' ON ' . $this->generateColumn($value[2]) . ' ' . $value[3] . ' ' . $this->generateColumn($value[4])); ```
} }

33
src/Repository/PlaceRepository.php
View File

@ -31,25 +31,30 @@ class PlaceRepository
//TODO: use Map and User instead of id //TODO: use Map and User instead of id
public function getRandomNPlaces(int $mapId, int $n, ?int $userId): array public function getRandomNPlaces(int $mapId, int $n, ?int $userId): array
{ {
if(!isset($userId)) { // anonymous single player if (!isset($userId)) { // anonymous single player
return $this->getRandomNForMapWithValidPano($mapId, $n); return $this->getRandomNForMapWithValidPano($mapId, $n);
} else { // authorized user or multiplayer game with selection based on what the host played before } else { // authorized user or multiplayer game with selection based on what the host played before
$unvisitedPlaces = $this->getRandomUnvisitedNForMapWithValidPano($mapId, $n, $userId); $unvisitedPlaces = $this->getRandomUnvisitedNForMapWithValidPano($mapId, $n, $userId);
if (count($unvisitedPlaces) == $n) {
bence commented 2021-05-08 22:39:27 +02:00
Outdated
Review
Copy Link

I would check if count($unvisitedPlaces) == $n before calling this function so we could save a function call and a DB query in a lot of cases.

if (count($unvisitedPlaces) == $n) {
    return $unvisitedPlaces;
}
I would check if `count($unvisitedPlaces) == $n` before calling this function so we could save a function call and a DB query in a lot of cases. ```php if (count($unvisitedPlaces) == $n) { return $unvisitedPlaces; } ```
return $unvisitedPlaces;
}
$oldPlaces = $this->getRandomOldNForMapWithValidPano($mapId, $n - count($unvisitedPlaces), $userId); $oldPlaces = $this->getRandomOldNForMapWithValidPano($mapId, $n - count($unvisitedPlaces), $userId);
return array_merge($unvisitedPlaces, $oldPlaces); return array_merge($unvisitedPlaces, $oldPlaces);
bence commented 2021-05-08 22:17:33 +02:00
Outdated
Review
Copy Link

This can be private now. And I don't remember why it has a parameter $exclude but I would remove that for a better understaning and create a local variable instead.

This can be `private` now. And I don't remember why it has a parameter `$exclude` but I would remove that for a better understaning and create a local variable instead.
} }
} }
//TODO: use Map instead of id //TODO: use Map instead of id
public function getRandomNForMapWithValidPano(int $mapId, int $n, array $exclude = []): array private function getRandomNForMapWithValidPano(int $mapId, int $n): array
bence commented 2021-05-08 22:18:04 +02:00
Outdated
Review
Copy Link

This can be removed too.

This can be removed too.
{ {
$places = []; $places = [];
$select = new Select(\Container::$dbConnection, 'places'); $select = new Select(\Container::$dbConnection, 'places');
$select->where('id', 'NOT IN', $exclude);
$select->where('map_id', '=', $mapId); $select->where('map_id', '=', $mapId);
$numberOfPlaces = $select->count(); $numberOfPlaces = $select->count();
$exclude = [];
for ($i = 1; $i <= $n; ++$i) { for ($i = 1; $i <= $n; ++$i) {
$place = $this->getRandomForMapWithValidPano($numberOfPlaces, $select, $exclude); $place = $this->getRandomForMapWithValidPano($numberOfPlaces, $select, $exclude);
@ -60,31 +65,31 @@ class PlaceRepository
return $places; return $places;
} }
private function getRandomForMapWithValidPano($numberOfPlaces, $select, array &$exclude, ?callable $pickRandomInt = null): ?Place private function getRandomForMapWithValidPano(int $numberOfPlaces, Select $select, array &$exclude, ?callable $pickRandomInt = null): ?Place
{ {
do { do {
$numberOfPlacesLeft = $numberOfPlaces - count($exclude); $numberOfPlacesLeft = $numberOfPlaces - count($exclude);
$place = $this->selectRandomFromDbForMap($numberOfPlacesLeft, $select, $exclude, $pickRandomInt); $place = $this->selectRandomFromDbForMap($numberOfPlacesLeft, $select, $exclude, $pickRandomInt);
if($place === null) { if ($place === null) {
// there is no more never visited place left // there is no more never visited place left
bence commented 2021-05-08 22:24:10 +02:00
Review
Copy Link

I wouldn't reformat this. There is no official coding guideline but I usually put a space after if, while, for, etc.

I wouldn't reformat this. There is no official coding guideline but I usually put a space after if, while, for, etc.
return null; return null;
} }
$panoId = $place->getFreshPanoId(); $panoId = $place->getFreshPanoId();
if($panoId === null) { if ($panoId === null) {
$exclude[] = $place->getId(); $exclude[] = $place->getId();
} }
} while($panoId === null); } while ($panoId === null);
return $place; return $place;
} }
private function selectRandomFromDbForMap($numberOfPlacesLeft, $select, array $exclude, ?callable $pickRandomInt): ?Place private function selectRandomFromDbForMap(int $numberOfPlacesLeft, Select $select, array $exclude, ?callable $pickRandomInt): ?Place
{ {
if($numberOfPlacesLeft <= 0) if ($numberOfPlacesLeft <= 0)
return null; return null;
if(!isset($pickRandomInt)) { if (!isset($pickRandomInt)) {
$randomOffset = random_int(0, $numberOfPlacesLeft - 1); $randomOffset = random_int(0, $numberOfPlacesLeft - 1);
} else { } else {
$randomOffset = $pickRandomInt($numberOfPlacesLeft); $randomOffset = $pickRandomInt($numberOfPlacesLeft);
@ -118,11 +123,11 @@ class PlaceRepository
// look for as many new places as possible but maximum $n // look for as many new places as possible but maximum $n
do { do {
$place = $this->getRandomForMapWithValidPano($numberOfUnvisitedPlaces, $selectUnvisited, $exclude); $place = $this->getRandomForMapWithValidPano($numberOfUnvisitedPlaces, $selectUnvisited, $exclude);
if(isset($place)) { if (isset($place)) {
$places[] = $place; $places[] = $place;
$exclude[] = $place->getId(); $exclude[] = $place->getId();
} }
} while(count($places) < $n && isset($place)); } while (count($places) < $n && isset($place));
return $places; return $places;
} }
@ -159,10 +164,10 @@ class PlaceRepository
}; };
// look for n - numberOfUnvisitedPlaces places // look for n - numberOfUnvisitedPlaces places
while(count($places) < $n) while (count($places) < $n)
{ {
$place = $this->getRandomForMapWithValidPano($numberOfOldPlaces, $selectOldPlaces, $exclude, $pickGaussianRandomInt); $place = $this->getRandomForMapWithValidPano($numberOfOldPlaces, $selectOldPlaces, $exclude, $pickGaussianRandomInt);
if(isset($place)) if (isset($place))
{ {
$places[] = $place; $places[] = $place;
$exclude[] = $place->getId(); $exclude[] = $place->getId();