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

Refactor game validity checks #636

Closed
Askaholic opened this issue Jul 28, 2020 · 2 comments · Fixed by #652
Closed

Refactor game validity checks #636

Askaholic opened this issue Jul 28, 2020 · 2 comments · Fixed by #652

Comments

@Askaholic
Copy link
Collaborator

When I added the coop validity checks initially, I added them all on the base Game class with a switch that checked the featured mod. However in hindsight, I should have made use of the existing object oriented design and put the coop checks in the CoopGame subclass. This would have the added benefit of decoupling the coop validity checks from the featured mod. We should also do a similar refactor for the "faf validity checks" as well.

Related code:

elif self.game_mode == FeaturedModType.COOP:

@46bit
Copy link
Contributor

46bit commented Aug 29, 2020

@Askaholic Hey. I had a look into this one and want to check the approach I'm thinking of.

The default Game.game_mode is FeaturedModType.FAF. But https://github.com/FAForever/server/blob/e4ca993/server/game_service.py#L155 instantiates a CustomGame for that game mode. As far as I can tell, Game isn't an abstract class but may as well be.

The logic in validate_game_settings is shared for all game modes, so it doesn't make sense to duplicate that. I think we're better off adding a validate_game_mode_settings method:

  • The existing logic in Game._validate_coop_game_settings would move to CoopGame.validate_game_mode_settings.
  • The existing logic in Game._validate_faf_game_settings would move to Game.validate_game_mode_settings or CustomGame.validate_game_mode_settings.

Remaining question is whether Game can practically be made abstract or not. Do you have any opinions on that?

@Askaholic
Copy link
Collaborator Author

Yea that sounds like what I had in mind, checks that happen for all games (like for unrated maps or mods etc) should happen on the base class, and checks that are specific to some other type of game like a coop game or a ladder game should happen on those classes. Right now there is a direct mapping between featured mod and game class, but that's going to change with TMM.

With the addition of TMM, we are going to (eventually) get rid of the ladder1v1 featured mod altogether, and just use the faf featured mod, but we will still use the LadderGame class for handling the matchmaker specific game logic. And there are also plans to generalize the logic regarding coop, so that we can more easily support coop games that are played using other featured mods, like nomads coop for example.

46bit added a commit to 46bit/faforever-server that referenced this issue Aug 29, 2020
This commit restructures the validity checks that are performed on
games. It starts taking advantage of the OOP setup of Game classes.
Child classes of Game can redefine the `_validate_game_mode_settings`
method to set custom game validity rules.
46bit added a commit to 46bit/faforever-server that referenced this issue Aug 31, 2020
This commit restructures the validity checks that are performed on
games. It starts taking advantage of the OOP setup of Game classes.
Child classes of Game can redefine the `validate_game_mode_settings`
method to set custom game validity rules.
46bit added a commit to 46bit/faforever-server that referenced this issue Sep 8, 2020
46bit added a commit to 46bit/faforever-server that referenced this issue Sep 9, 2020
This commit restructures the validity checks that are performed on
games. It starts taking advantage of the OOP setup of Game classes.
Child classes of Game can redefine the `validate_game_mode_settings`
method to set custom game validity rules.
Askaholic pushed a commit that referenced this issue Sep 9, 2020
This commit restructures the validity checks that are performed on
games. It starts taking advantage of the OOP setup of Game classes.
Child classes of Game can redefine the `validate_game_mode_settings`
method to set custom game validity rules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants