Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 11 top rating user title #115

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Fezzzi
Copy link
Collaborator

@Fezzzi Fezzzi commented Mar 13, 2020

Closes #11

@Fezzzi Fezzzi added enhancement New feature or request needs styling labels Mar 13, 2020
@Fezzzi Fezzzi self-assigned this Mar 13, 2020
@jkotrlik jkotrlik had a problem deploying to foosball-dev-pr-115 March 13, 2020 15:10 Failure
@Fezzzi Fezzzi force-pushed the issue-11_top-rating-user-title branch from 8c23550 to ae1e0a2 Compare March 19, 2020 17:50
@Fezzzi Fezzzi force-pushed the issue-11_top-rating-user-title branch from ae1e0a2 to d800d16 Compare March 27, 2020 09:08
@Fezzzi Fezzzi requested a review from littlewhywhat March 27, 2020 09:41
@Fezzzi Fezzzi closed this Mar 27, 2020
@Fezzzi Fezzzi reopened this Mar 27, 2020
@jkotrlik jkotrlik temporarily deployed to foosball-dev-pr-115 March 27, 2020 09:50 Inactive
@Fezzzi
Copy link
Collaborator Author

Fezzzi commented Mar 27, 2020

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 Fezzzi force-pushed the issue-11_top-rating-user-title branch from d800d16 to 639dd1a Compare July 10, 2020 11:44
<ListItem Display="grid" Column="2fr 1fr 2fr">
<ListItem display="grid" column="2fr 1fr 2fr">
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Comment on lines +51 to +52
display: ${props => props.display};
grid-template-columns: ${props => props.column};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the sake of consistency


const kingId = playersLast[0].id
let matchFound = false
for (const match of matchesLast) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@littlewhywhat littlewhywhat Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe if you could a bit elaborate on those 3 steps... The way i see how it can be computed is smth like that:

  1. King may become a such by playing a match or by somebody else playing a match (and losing rating)
  2. I think we have a snapshot of players ratings in their matches
  3. 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? 🤷‍♂️
  4. 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?...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrk-salsita @novellizator any thoughts? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Fezzzi force-pushed the issue-11_top-rating-user-title branch from 639dd1a to 386ab96 Compare July 24, 2020 08:54
@Fezzzi Fezzzi assigned littlewhywhat and unassigned Fezzzi Aug 7, 2020
@jkotrlik jkotrlik temporarily deployed to foosball-dev-pr-115 August 7, 2020 09:19 Inactive
@Fezzzi Fezzzi assigned Fezzzi and unassigned littlewhywhat Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement: Top Ratings
3 participants