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

[WIP] Refactor Tourney classes #16090

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

Conversation

isaacl
Copy link
Member

@isaacl isaacl commented Sep 20, 2024

  • Move recently created complex logic into its own object
  • Add a deterministic tourney stagger method
  • Add stagger to Plan objects, rather than Tournaments to be more easily testable.

@isaacl isaacl changed the title [WIP] Deterministic tourney scheduling with better stagger. [WIP] Refactor Tourney classes Sep 21, 2024
@isaacl isaacl force-pushed the tourneyDeterministicStaggers branch 4 times, most recently from d49e666 to aa748a8 Compare September 22, 2024 21:32
@isaacl
Copy link
Member Author

isaacl commented Sep 22, 2024

This probably works, though I haven't tested. However it's kind of a hot mess of code. I'm struggling to figure out how to simplify. I don't really need a TreeSet for the tiny list of tourneys, but even without that structure, the code looks the same.

This whole change is probably overkill, but the idea was:

  • clean up and segregate the logic I've been dumping into tourney classes
  • uniform random staggers are not particularly good at their job -- they cause tourneys to be closer together than might be expected.

Still, it feels like a lot of verbose code for a relatively small issue :-\ so not sure what to do.

- Move recently created complex logic into its own object
- Add a deterministic tourney stagger method
- Add stagger to Plan objects, rather than Tournaments to
  be more easily testable.
@ornicar
Copy link
Collaborator

ornicar commented Sep 24, 2024

It's a lot of tricky code to review. I did read it quickly and found nothing alarming. I deleted my local tournaments and let it re-create them. They look fine, and the deterministic stagger is certainly nice.

Surely there are some bugs lurking, and it's ok to find them later, in prod.

@ornicar ornicar marked this pull request as ready for review September 24, 2024 21:07
@isaacl
Copy link
Member Author

isaacl commented Sep 25, 2024

I’ll be adding tests of the new code before it’s ready to land. Will follow up.

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