Skip to content

Frivolity for Oldest Champion Team - #425

Merged
dumbmatter merged 4 commits into
zengm-games:masterfrom
maxkarnold:maxkarnold
Jul 5, 2022
Merged

Frivolity for Oldest Champion Team#425
dumbmatter merged 4 commits into
zengm-games:masterfrom
maxkarnold:maxkarnold

Conversation

@maxkarnold

Copy link
Copy Markdown
  • Not sure if you wanted tests, since there aren't any currently that I can see.
  • Right now the default value if avgAge is undefined is 0, which I don't think should happen since it should filter out any seasons with no avgAge.
  • Right now it's rounded to one decimal place, since that's what is on the roster screen, so I figured that would be best.
  • avgAge is the main sortParam, but I included winp in the unlikely case that there is the exact same avgAge value before rounding.
  • I noticed on the "worst championship teams", the title says "worst champion team". Not sure exactly why you did that, if you wanted to keep that I can change it to "oldest champion team" for the new frivolity.

@dumbmatter dumbmatter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"worst champion team" was probably a typo, please fix it in this PR :)

Want to do youngest championship teams in this PR too?

Another thing I noticed - this doesn't work for historical data in real player leagues, because it doesn't have avgAge computed. I should probably fix that myself, cause I can put it in the real player data generator which is not part of this repo.

Comment thread src/worker/views/frivolitiesTeamSeasons.ts Outdated
Comment thread src/worker/views/frivolitiesTeamSeasons.ts Outdated
@maxkarnold

Copy link
Copy Markdown
Author

"worst champion team" was probably a typo, please fix it in this PR :)

Want to do youngest championship teams in this PR too?

Another thing I noticed - this doesn't work for historical data in real player leagues, because it doesn't have avgAge computed. I should probably fix that myself, cause I can put it in the real player data generator which is not part of this repo.

I saw that all of your other frivolities, you just had "Oldest insert frivolity here" That makes sense, since you can just sort in reverse order and get the youngest champions. (Will definitely fix that typo as well)

@dumbmatter

Copy link
Copy Markdown
Member

I saw that all of your other frivolities, you just had "Oldest insert frivolity here" That makes sense, since you can just sort in reverse order and get the youngest champions

You can only do that if there are fewer than 100 seasons in the league, but frivolities are most interesting for very large leagues.

And for the other current "Oldest" frivolities a "Youngest" wouldn't be interesting, but here it is.

@maxkarnold

maxkarnold commented Jul 5, 2022

Copy link
Copy Markdown
Author

I saw that all of your other frivolities, you just had "Oldest insert frivolity here" That makes sense, since you can just sort in reverse order and get the youngest champions

You can only do that if there are fewer than 100 seasons in the league, but frivolities are most interesting for very large leagues.

And for the other current "Oldest" frivolities a "Youngest" wouldn't be interesting, but here it is.

Oh ok, haha. I guess I haven't played the game long enough to realize that.

I guess youngest former player sounds kinda stupid. And youngest to play in a game might also not be as interesting. I'll add the youngest championship 😄 .

@maxkarnold
maxkarnold requested a review from dumbmatter July 5, 2022 01:47
@dumbmatter
dumbmatter merged commit fdf48e2 into zengm-games:master Jul 5, 2022
@dumbmatter

Copy link
Copy Markdown
Member

Thanks! There were a couple other things I wanted to change a bit, but I just did them myself in 1cfa754 and c85cecc.

I want to get avgAge working for historical seasons now, hopefully I'll do that soon before I release it otherwise I'm sure people will complain about it lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants