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
Member

Upgraded selection of places for the game

  • Priotrizing places never played first
  • Added higher probability for selection of places that have been visited in the longest time
**Upgraded selection of places for the game** - Priotrizing places never played first - Added higher probability for selection of places that have been visited in the longest time
balazs added 13 commits 2021-05-06 20:46:44 +02:00
replaced left join with inner join for older places search
All checks were successful
default-pipeline default-pipeline #169
8136273b33
renamed the random int generator function
All checks were successful
default-pipeline default-pipeline #170
fb7c0e7a5c
moved saving to UserPlayedPlace to seperate function
Some checks failed
default-pipeline default-pipeline #171
7143c7ec63
removed mistakenly added parameter
All checks were successful
default-pipeline default-pipeline #172
886bd02f88
refactored function and variable names, and replaced variables in inner scope
All checks were successful
default-pipeline default-pipeline #173
b2535ad78a
handling of deleting places or user updated
All checks were successful
default-pipeline default-pipeline #174
8b3c95bdc7
fixing comments
All checks were successful
default-pipeline default-pipeline #175
8d8074977b
specified explicit type in function parameter
All checks were successful
default-pipeline default-pipeline #176
c626e36bbb
removed unnecessary comment
All checks were successful
default-pipeline default-pipeline #177
899817a853
balazs requested review from bence 2021-05-06 20:46:53 +02:00
bence started working 2021-05-08 20:25:24 +02:00
bence reviewed 2021-05-08 21:35:04 +02:00
@ -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]));
Owner

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])); ```
bence reviewed 2021-05-08 21:39:03 +02:00
@ -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) {
Owner

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) { ```
bence reviewed 2021-05-08 22:17:33 +02:00
@ -31,2 +41,4 @@
}
//TODO: use Map instead of id
public function getRandomNForMapWithValidPano(int $mapId, int $n, array $exclude = []): array
Owner

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.
bence reviewed 2021-05-08 22:18:04 +02:00
@ -34,2 +46,4 @@
$places = [];
$select = new Select(\Container::$dbConnection, 'places');
$select->where('id', 'NOT IN', $exclude);
Owner

This can be removed too.

This can be removed too.
bence reviewed 2021-05-08 22:22:05 +02:00
@ -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
Owner
int $numberOfPlaces, Select $select

```php int $numberOfPlaces, Select $select ```
bence reviewed 2021-05-08 22:24:10 +02:00
@ -51,3 +72,3 @@
$panoId = $place->getFreshPanoId();
if ($panoId === null) {
if($panoId === null) {
Owner

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.
bence reviewed 2021-05-08 22:39:27 +02:00
@ -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);
Owner

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; } ```
bence stopped working 2021-05-08 22:42:02 +02:00
2h 16min 38s
Owner

Really good feature, thank you for implementing this! I just had some minor findings.

Really good feature, thank you for implementing this! I just had some minor findings.
balazs added 1 commit 2021-05-08 23:37:50 +02:00
implemented review findings
All checks were successful
default-pipeline default-pipeline #188
3f8311d708
bence approved these changes 2021-05-09 10:54:42 +02:00
balazs merged commit 216d30329f into develop 2021-05-09 10:58:54 +02:00
balazs deleted branch feature/avoid-repeating-places-in-game 2021-05-09 10:59:05 +02:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Total Time Spent: 2 hours 16 minutes
bence
2 hours 16 minutes
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#38
No description provided.