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

Decompose GameState into sub-records #1510

Merged
merged 18 commits into from
Sep 11, 2023
Merged

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Sep 10, 2023

Towards #872

Previously, the GameState record had 41 toplevel members. It now has 22. Logical grouping of the fields makes it easier to peruse the code and paves the way to split State.hs into smaller modules. Some functions now may even be able to operate exclusively on subsets of the game state, rather than having to pass in GameState as a whole.

There is potential to go even farther, by extracting view-related and robot-related members to their own records, but I figured I'd pursue this refactoring incrementally.

@kostmo kostmo added the Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality. label Sep 10, 2023
@kostmo kostmo marked this pull request as draft September 10, 2023 06:57
@kostmo kostmo marked this pull request as ready for review September 11, 2023 00:40
@kostmo kostmo requested a review from byorgey September 11, 2023 00:40
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

This is great! It's a good idea in general, and I like the subcategories you have chosen to organize things. There are just two improvements I'd like to see:

  • New subrecord types like Temporal, Landscape, etc. should be exported (just the types, not their constructors or record fields)
  • We should put some effort into organizing the generated Haddock documentation. At the very least we should put each of the new subrecords + their lenses under a separate subheading in the export list.

@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Sep 11, 2023
@mergify mergify bot merged commit bb31126 into main Sep 11, 2023
9 checks passed
@mergify mergify bot deleted the refactor/decompose-gamestate branch September 11, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants