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 PropertyReader signature smaller #743

Open jkomoros opened 4 years ago

jkomoros commented 4 years ago

Discovered while working on #737.

PropertyReader (and the rest of the family) are just interfaces, and have to be implemented by the user of the package. That means that it's hard to have little convenience methods like TypeOf(propName) PropertyType and MustInt() int (the latter as an example that doesn't return an error, just a zero-value int if it's invalid), because they have to be generated by the user.

Obviously boardgame codegen mitigates this somewhat but it still feels less than ideal, for example if a new convenience method wants to be added then game package authors have to regenerate code.

Really the only required properties are Props(), Prop, SetProp, and ConfigureProp (right? the signatures are complex). Everything else can be implemented in terms of those.

Then you'd have a PropertyGetter (etc) concrete struct that wraps a PropertyReader and does the convenience methods on it.

The main thing to check is performance. Currently the PropetyReader implementation code uses switch statements for the given propnames; this would run more code and have more indirection. But the most expensive thing is likely the reflection in the (very) old system, so this should probably be fine--just check.

jkomoros commented 4 years ago

To test the performance implications, can just leave all of the getters/setters on PropertyReader, but introduce a PropertyGetter that ignores those getters.

jkomoros commented 4 years ago
jkomoros commented 4 years ago

Typically nearly all use of property readers will be via the wrapped values. Should part of the signature of Reader() be to return a convenience-wrapped version of the core getter, so you could use it like state.Reader().Getter().MustInt("Score")?

jkomoros commented 4 years ago

Actually, it's probably best to not expose more notions externally than necessary. Instead, it's best to have a readerhelper package:

package readerhelper

//CorePropertyReader is the minimal internal interface that your struts must implement to use the Core helper.
type CorePropertyReader interface {
  Props() map[string]boardgame.PropertyType
  Prop(name string) (interface{}, error)
}

type CorePropertyReadSetter interface {
  CorePropertyReader
  SetProp(name string, val interface{}) error
}

func NewReader(core CorePropertyReader) boardgame.PropertyReader {
  return reader{core}
}

//reader wraps a minimal CorePropertyReader, passing through its two main methods and extending with all of the other ones to extend the interface to complete all of boardgame.PropertyReader
type reader struct {
  CorePropertyReader
}

func NewReadSetter(core CorePropertyReadSetter) boardgame.PropertyReadSetter {
  return readSetter{reader{core}}
}

type readSetter struct {
  reader
}

Add tests in readerhelper to ensure that Core adheres to PropertyReader interface, to quickly catch where its missing methods.

jkomoros commented 4 years ago

Currently the generated PropertyReaders from codegen have their meat in the specific property accessors (e.g. IntProp), with Prop being basically just a multiplexer to those. It's not really clear that reducing it down to this core readsetter is better just to get a few convenience methods.