search

jkomoros / boardgame

An in-progress framework in golang to easily build boardgame Progressive Web Apps
Apache License 2.0
31 stars 4 forks source link

Consider making PlayerIndex be a struct #763

Closed jkomoros closed 4 years ago

jkomoros commented 4 years ago

Originally captured in #755. Copying the reasoning here:

Another option: make PlayerIndex an opaque type, that's actually a struct. When serialized to/from JSON it would pretend to be just an int, but in the actual live structs it would be a struct that has a pointer to the (possibly nil) state it's in. That would disallow arithmetic on it naturally, and also let things like Next / Prev operate in place. We use playerIndex for a LOT of things in other packages, how much would need to change?

Other interesting things: AdminPlayerIndex and ObserverPlayerIndex would become pointers (that serialize to -1 and -2 in JSON)

jkomoros commented 4 years ago

PlayerIndexes are used in a LOT of places, e.g StatePropertyRef, etc. If they're an interface, then you'd be passing around the actual copy a lot, and it could be modified--which probably isn't what you want, because you pass the PlayerIndex to lots of other code and who knows what it'll do it with it. ... I guess you'd do the same ImmutablePlayerIndex/PlayerIndex split. But that... seems needlessly complicated.

jkomoros commented 4 years ago

There could be another type, a ValidPlayerIndex, which is just type ValidPlayerIndex PlayerIndex, which communicates to the system, please validate that player index. Or do a struct tag that tells the system, this player index is one that needs to be kept valid.

The reasoning behind having it be a struct is 1) you can mix in the state pointer so you don't have to pass in state to all of the methods, and 2) you can make it so that arithmetic on it is prevented, to make it less likely that someone in userland accidentally trips themselves up.

The first seems kind of like an anti-pattern, because in many cases they're not in the context of a given State, and you wouldn't want others to modify it. It seems like there are easier ways to avoid the second.

jkomoros commented 4 years ago

Actually, this is unneccesary. Just mark clearly in the type documentation to not do arithmetic on them directly, and have a ValidOrNext() that everything uses to verify that the index is in a good place whenever it needs to get an index that might not be valid.