SatelliteData interface should be optional

Hexworks / mixite

A GUI agnostic hexagonal grid library. Supports a multitude of grid layouts including hexagonal, triangular, rectangular and more.

Apache License 2.0

191 stars 31 forks source link

SatelliteData interface should be optional #52

Open drekbour opened 4 years ago

drekbour commented 4 years ago

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 needmovementCostorisPassableorisOpaquethen 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 commented 4 years ago

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)

adam-arold commented 4 years ago

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?

drekbour commented 4 years ago

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!

adam-arold commented 4 years ago

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

drekbour commented 4 years ago

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.