mixite icon indicating copy to clipboard operation
mixite copied to clipboard

SatelliteData interface should be optional

Open drekbour opened this issue 5 years ago • 5 comments

As an API interface SatelliteData is actually a very opinionated choice of data. There is no hard requirement for any enforced supertype to the storage data (e.g. the core class, HexagonalGridImpl, doesn't use it even once). SatelliteDataexists so Mixite can supply some useful algorithms via the extendedHexagonalGridCalculator. If the user doesn't need movementCostorisPassableorisOpaquethen it's just cruft (note for examplemovementCost` is not used anywhere in the code base!).

Ergo, SatelliteData interface should be optional.

However, those additional utilities are very useful and key part of the value of this library.

drekbour avatar Nov 24 '19 18:11 drekbour

first thoughts:

  • Remove calculator from builder
  • Split single calculator into seperate items based on actual data requirement (several methods don't need SatelliteData at all)
    • It's possible to avoid the interface altogether by supplying a function. E.g. isVisible( from:Hex, to:Hex, isOpaque: Hexagon->Boolean)

drekbour avatar Nov 24 '19 18:11 drekbour

So this is some very old code, that's why these weird things are in the codebase. I agree that this needs to be cleaner, but I didn't work on this for a while now so I need to wrap my head around it in order to be able to respond to this. What we could do is to have multiple interfaces and one of them has Nothing in place of the satellite data class. I'll take a look probably at the end of this year. How would the split look like in your opinion?

adam-arold avatar Nov 24 '19 18:11 adam-arold

Nor sure as I've only been a Kotlin programmer for about 4 hours (literally). Theres a lot of nice 'infix' pimp-my-api stuff could be done but I don't know how that works in mpp etc. So much to learn!

drekbour avatar Nov 24 '19 20:11 drekbour

It is a good idea, but we have to maintain API compatibility with Java. 😢

adam-arold avatar Nov 24 '19 20:11 adam-arold

I don't think it's mutually exclusive. if the calculators are simple standalone classes then a "clever" infix layer that glues a them onto suitable Hexagon<T> is an addition for Kotlin users. Too much for me to comment more on till I've gotten the hang of KT a bit more but what I will PR at some point is removing T: SatelliteData from all classes except that calculator.

drekbour avatar Nov 24 '19 20:11 drekbour