Add GameDataState (extract first TechTracker from GameData) #13714
Conversation
| @Getter private transient TechTracker techTracker = new TechTracker(this); | ||
| private final IGameLoader loader = new TripleA(); | ||
| private History gameHistory = new History(this); | ||
| private StateGameData stateGameData = new StateGameData(this); |
There was a problem hiding this comment.
StateGameData -> GameDataState
or
| private StateGameData stateGameData = new StateGameData(this); | |
| private transient StateGameData state = new StateGameData(this); |
game-app/game-core/src/main/java/games/strategy/engine/data/statedata/StateGameData.java
Outdated
Show resolved
Hide resolved
game-app/game-core/src/main/java/games/strategy/engine/data/statedata/StateGameData.java
Outdated
Show resolved
Hide resolved
game-app/game-core/src/main/java/games/strategy/engine/data/GameData.java
Outdated
Show resolved
Hide resolved
game-app/game-core/src/main/java/games/strategy/engine/data/statedata/StateGameData.java
Outdated
Show resolved
Hide resolved
|
per discussion:
|
frigoref
changed the title
2. Replace History with TechTracker in GameDataState extracted from GameData 3. Add Getter/Setter for GameHistory in GameData 4. postDeSerialize to instantiate GameDataState
| // Don't ensure the lock is held when getting the history. | ||
| // History operations often acquire the write lock, and we can't acquire the write lock if we | ||
| // have the read lock. | ||
| @Setter @Getter private History gameHistory = new History(this); |
There was a problem hiding this comment.
I notice the new @Setter + @Getter. I think it was actually better with direct variable access before.
If we go with @Getter + @Setter, we should arguably use the least scope possible (eg: private/protected/package default)
Overall, I'd say direct variable access is to be preferred, namely because of inheritance.
(1)With direct variable access, child classes can't make unexpected changes to unrelated methods by overriding setters
(2) the code is more straight forward. When seeing a method access, a person needs to trace that down to the annotation, then confirm any child classes and any overrides. With the direct variable usage, there is no indirection.
There was a problem hiding this comment.
That is in general correct, but it is pre-step for another PR to get rid of the methods after renaming them. I did not want to include it in this PR as it would have effected >5 other classes.
| // History operations often acquire the write lock, and we can't acquire the write lock if we | ||
| // have the read lock. | ||
| @Setter @Getter private History gameHistory = new History(this); | ||
| private GameDataState state = new GameDataState(this); |
There was a problem hiding this comment.
| private GameDataState state = new GameDataState(this); | |
| private transient GameDataState state = new GameDataState(this); |
There was a problem hiding this comment.
If this value is transient, then we can remove also remove the null check in line 130 and always create a new instance when the game data object loads.
There was a problem hiding this comment.
Was it intentional to leave off transient?
There was a problem hiding this comment.
Yes, it was intentional. I remove the handling in readObject as it is already in postDeSerialized.
See new commit aae6c5c.
| @Getter @Setter private transient TechTracker techTracker; | ||
|
|
||
| public GameDataState(GameData gameData) { | ||
| techTracker = new TechTracker(gameData); |
There was a problem hiding this comment.
Are next iterations going to remove gameData as a parameter here? Specifically we would just pull out whatever data we need and pass that directly to TechTracker.
Removing references to GameData is a good win. Looks like we are on a path to that here 👍
There was a problem hiding this comment.
I have added a todo comment to the class.
See new commit aae6c5c.
2. Remove readObject handling of state (already in postDeSerialize)
|
looks good; @frigoref, back to you to do a one last look over before merge. |
2 participants
No description provided.