Issue 11 top rating user title #115
Conversation
Fezzzi
force-pushed
the
issue-11_top-rating-user-title
branch
from
8c23550
to
ae1e0a2
Compare
Fezzzi
force-pushed
the
issue-11_top-rating-user-title
branch
from
ae1e0a2
to
d800d16
Compare
|
There are some issues with loading data, I will fix that with rebase after merging #27 so lets focus on that for now and ignore this one... |
Fezzzi
force-pushed
the
issue-11_top-rating-user-title
branch
from
d800d16
to
639dd1a
Compare
| <ListItem Display="grid" Column="2fr 1fr 2fr"> | ||
| <ListItem display="grid" column="2fr 1fr 2fr"> |
There was a problem hiding this comment.
Just for the sake of consistency
| @@ -52,6 +51,6 @@ export const getStatisticsForPlayer = createSelector( | |||
|
|
|||
| export const getRatingHistoryGraphForPlayer = createSelector( | |||
| getLastMatchesForPlayer, | |||
| getPlayer, | |||
| (state, playerId) => state.players.find(player => player.id === playerId), | |||
There was a problem hiding this comment.
I had to change this due to (probably) circular dependency in selectors between matches-selectors and players-selectors:
Error: Selector creators expect all input-selectors to be functions, instead received the following types: [function, undefined]
An ideas about alternative solution are welcome!
There was a problem hiding this comment.
usually the solution for A <-> B is to add C :) so A <- C, B <- C :) so I guess some selectors could be put outside of matches/players ones
There was a problem hiding this comment.
I thought about it for a bit and decided that conceptually, there is no need for third group of selectors as they are all still selectors of players and matches. I tried to reorganize and refactor selectors and computations but it is hard to decide when a thing working with both players and matches should be labeled as players-selector and when as matches-selector. I used as a rule of a thumb some similaties (such as all getXXXForPlayer selectors are together) and general functionalities of things (like computeKingStreakDuration seems more of players-computation rather then matches one).
What do you think about the current design and what would you improve/change @littlewhywhat ?
There was a problem hiding this comment.
if it works and there is no unnecessary code duplication (it can be accepted sometimes though), I am fine with it :) I was mainly answering your question) I don't mind that much about organizing things on a such low level... I think in this case, one (or at least me) would use IDE tools to navigate through those function anyway. And there are probably hundreds of way to organize these functions and none of them is probably perfect 🤷♂️ so if it makes sense to you it's good 👍
| display: ${props => props.display}; | ||
| grid-template-columns: ${props => props.column}; |
There was a problem hiding this comment.
Just for the sake of consistency
|
|
||
| const kingId = playersLast[0].id | ||
| let matchFound = false | ||
| for (const match of matchesLast) { |
There was a problem hiding this comment.
looks a bit like black magic to me :D i guess idea is to go backwards until we find some match (moment in time) where the king became a king, right?
There was a problem hiding this comment.
maybe if you could a bit elaborate on those 3 steps... The way i see how it can be computed is smth like that:
- King may become a such by playing a match or by somebody else playing a match (and losing rating)
- I think we have a snapshot of players ratings in their matches
- If King became a such by other losing some rating then it seems easy - just find a match where someone has a bigger rating then King's (assuming you update King's rating if it changes). I guess it's right? 🤷♂️
- If King became a such by gaining some rating then we need somehow to realize that the previous King has lower rating... Previous King can be or not in the match - so I am afraid we need to compare somehow to all other players here (or at least the second biggest at that moment - has to be computed somehow) and understand if this won match is this deal-breaker match...
Probably first 3 I somehow see in the code below but definitely missing 4th point... maybe I am missing something in my conclusions?...
There was a problem hiding this comment.
in any case not a trivial algorithm definitely needs more docs/comments or maybe we should just solve it by adding something simple in the BE instead
There was a problem hiding this comment.
@petrk-salsita @novellizator any thoughts? :)
There was a problem hiding this comment.
I added some brief explanation of the algorithm and it here as well:
First, we create a hash table of players and their current scores
Then we iterate through matches from the latest, recompute scores of affected players and compare
More preciselly:
1. Recompute king's score in case he played the match, note whether he won the match
2. Recompute scores of all (other) players in the match and test if any of them tops king
3. Otherwise, compare his new score with scores of all other players,
in case the king played the match and won (thus had smaller score before).
How you described it is basically the way it is implemented, looping over players affected by the match (your 3.) is 2. in the docs (lines 84-87) and looping over all players in case King won the match (your 4.) is 3. in the docs (lines 89-93)
Fezzzi
force-pushed
the
issue-11_top-rating-user-title
branch
from
639dd1a
to
386ab96
Compare
Closes #11