YetAnotherSpieskowcy / Carcassonne-Engine

Other
0 stars 1 forks source link

Fix PlacedTile immutability assumption in board.DeepClone() #118

Closed Jackenmen closed 1 month ago

Jackenmen commented 1 month ago

Before meeple removal was added (#55), PlacedTile was considered immutable. This already needed to be fixed in Game.PlayTurn() (#106) but today I found another place where this assumption was made but is no longer correct.

Ideally, I would actually prefer to remove the mutation of PlacedTile by updating RemoveMeeple() to modify a clone of the tile but that is not as easy to do - we'd need to update both board.tiles and board.tilesMap (the former requires search) as well as ensure that any code looking up the tile once (probably just board.ScoreMeeples()) is changed to look the tile up again if it performed meeple removal. In short: not worth it.

The key part is https://github.com/YetAnotherSpieskowcy/Carcassonne-Engine/pull/118/commits/571610c23c82ba731c7779520d5dbde47cb3140f The other commits are adding tests and improving existing game/board split to make those tests viable - having to remove the meeples on the game side of things is a weird design choice.