feature/avoid-repeating-places-in-game #38
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "feature/avoid-repeating-places-in-game"
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?
Upgraded selection of places for the game
@ -306,1 +336,3 @@
});
foreach($joins as $value) {
list($joinQueryFragment, $paramsFragment) = $this->generateTable($value[1], true);
array_push($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:@ -304,3 +336,1 @@
array_walk($joins, function (&$value, $key) {
$value = $value[0] . ' JOIN ' . $this->generateTable($value[1], true) . ' ON ' . $this->generateColumn($value[2]) . ' ' . $value[3] . ' ' . $this->generateColumn($value[4]);
});
foreach($joins as $value) {
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.@ -31,2 +41,4 @@
}
//TODO: use Map instead of id
public function getRandomNForMapWithValidPano(int $mapId, int $n, array $exclude = []): array
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.@ -34,2 +46,4 @@
$places = [];
$select = new Select(\Container::$dbConnection, 'places');
$select->where('id', 'NOT IN', $exclude);
This can be removed too.
@ -45,3 +62,2 @@
//TODO: use Map instead of id
public function getRandomForMapWithValidPano(int $mapId, array $exclude = []): Place
private function getRandomForMapWithValidPano($numberOfPlaces, $select, array &$exclude, ?callable $pickRandomInt = null): ?Place
@ -51,3 +72,3 @@
$panoId = $place->getFreshPanoId();
if ($panoId === null) {
if($panoId === null) {
I wouldn't reformat this. There is no official coding guideline but I usually put a space after if, while, for, etc.
@ -31,0 +35,4 @@
return $this->getRandomNForMapWithValidPano($mapId, $n);
} else { // authorized user or multiplayer game with selection based on what the host played before
$unvisitedPlaces = $this->getRandomUnvisitedNForMapWithValidPano($mapId, $n, $userId);
$oldPlaces = $this->getRandomOldNForMapWithValidPano($mapId, $n - count($unvisitedPlaces), $userId);
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.Really good feature, thank you for implementing this! I just had some minor findings.