Split Server #3652
Split Server #3652
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3652 +/- ##
============================================
- Coverage 23.78% 23.63% -0.15%
+ Complexity 4878 4811 -67
============================================
Files 2202 2204 +2
Lines 242345 242430 +85
Branches 45419 45423 +4
============================================
- Hits 57632 57294 -338
- Misses 183230 183672 +442
+ Partials 1483 1464 -19
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This largely looks good, and should help simplify my connection changes which is even better. I've got three questions, and made a few comments otherwise.
- Is there a reason that server::getGameManager isn't used in cases where we'd otherwise be passing both the server and the game manager? (I've mostly noticed this in Commands)
- For Commands, would swapping to using an AbstractGameManagerCommand extending ServerCommand make more sense than the new setup? This also may be nullified by the previous.
- IGame and IGameManager... are both useless interfaces currently. Can we please avoid increasing the time waste they'll cause in the short term (that's why most of the MM interfaces have been torn out), and add them when they are actually useful to have (and so how we can only have it include necessary methods)?
Co-authored-by: Justin Bowen <[email protected]>
Co-authored-by: Justin Bowen <[email protected]>
|
I'll start with point 3. Unused is not the same as useless. It is easier to build extensibility into the design than to try to add it later. I haven't yet assessed how damaging the removal of IBoard is to my desire to implement multi-map engagements, but it has certainly added a significant barrier. Also as Watford pointed out when the question of the interfaces came up last year, they make it easier to write good unit tests. Having stated that, the reason both the Server and the GameManager instance are passed to the server command constructors is that they are specific to the standard BT GameManager, and specifying which specific implementation they need is safer than typecasting an instance of IGameManager. |
|
Merging so that I can migrate the third connection rework to this, and prevent further merge conflicts. |
There's an old adage that the way to eat an elephant is one bite at a time. But before that the carcass needs to be processed, which is what I've tried to do here by separating the code that deals with the rules of the game from the actual server code. Code that doesn't deal with establishing and maintaining connections is now in GameManager.java. The use of an IGameManager provides a level of abstraction to make it easier to add support for other game systems such as BattleForce or Alpha Strike to use the same server code. Likewise, a partially rebuilt IGame is used for the small amount the server still has to do with Game. I didn't change any code other than what was necessary to support the split.
I tested it by starting a new game, loading a saved game, loading a scenario, and using manual AMS resolution to make sure CFRs still work correctly.
Though this only reduces the current Server.java by about 1,200 lines of code, it is the most logical split by function, and hopefully just the beginning of the long refactor.