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 9 Commits 14 Files Changed 10 +397 -35
balazs commented 2021-05-06 20:46:44 +02:00
Member
Copy Link

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
Details
8136273b33
renamed the random int generator function
All checks were successful
default-pipeline default-pipeline #170
Details
fb7c0e7a5c
moved saving to UserPlayedPlace to seperate function
Some checks failed
default-pipeline default-pipeline #171
Details
7143c7ec63
removed mistakenly added parameter
All checks were successful
default-pipeline default-pipeline #172
Details
886bd02f88
refactored function and variable names, and replaced variables in inner scope
All checks were successful
default-pipeline default-pipeline #173
Details
b2535ad78a
handling of deleting places or user updated
All checks were successful
default-pipeline default-pipeline #174
Details
8b3c95bdc7
fixing comments
All checks were successful
default-pipeline default-pipeline #175
Details
8d8074977b
specified explicit type in function parameter
All checks were successful
default-pipeline default-pipeline #176
Details
c626e36bbb
removed unnecessary comment
All checks were successful
default-pipeline default-pipeline #177
Details
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
src/Database/Query/Select.php
@ -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]));
bence commented 2021-05-08 21:35:04 +02:00
Owner
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])); ```
bence reviewed 2021-05-08 21:39:03 +02:00
src/Database/Query/Select.php Outdated
@ -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) {
bence commented 2021-05-08 21:39:03 +02:00
Owner
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) { ```
bence reviewed 2021-05-08 22:17:33 +02:00
src/Repository/PlaceRepository.php Outdated
@ -31,2 +41,4 @@
}
//TODO: use Map instead of id
public function getRandomNForMapWithValidPano(int $mapId, int $n, array $exclude = []): array
bence commented 2021-05-08 22:17:33 +02:00
Owner
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.
bence reviewed 2021-05-08 22:18:04 +02:00
src/Repository/PlaceRepository.php Outdated
@ -34,2 +46,4 @@
$places = [];
$select = new Select(\Container::$dbConnection, 'places');
$select->where('id', 'NOT IN', $exclude);
bence commented 2021-05-08 22:18:04 +02:00
Owner
Copy Link

This can be removed too.

This can be removed too.
bence reviewed 2021-05-08 22:22:05 +02:00
src/Repository/PlaceRepository.php Outdated
@ -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
bence commented 2021-05-08 22:22:05 +02:00
Owner
Copy Link
int $numberOfPlaces, Select $select
```php int $numberOfPlaces, Select $select ```
bence reviewed 2021-05-08 22:24:10 +02:00
src/Repository/PlaceRepository.php
@ -51,3 +72,3 @@
$panoId = $place->getFreshPanoId();
if ($panoId === null) {
if($panoId === null) {
bence commented 2021-05-08 22:24:10 +02:00
Owner
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.
bence reviewed 2021-05-08 22:39:27 +02:00
src/Repository/PlaceRepository.php Outdated
@ -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);
bence commented 2021-05-08 22:39:27 +02:00
Owner
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; } ```
bence worked for 2h 16min 38s 2021-05-08 22:42:02 +02:00
bence commented 2021-05-08 22:43:45 +02:00
Owner
Copy Link

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
Details
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.
Reviewers
No Reviewers
bence
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
balazs bence system-user-ci
No Assignees
2 Participants
Notifications
Total Time Spent: 2 hours 16 minutes
bence
2 hours 16 minutes
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: esoko/mapguesser#38
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?