Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SatelliteData interface should be optional #52

Open
drekbour opened this issue Nov 24, 2019 · 5 comments
Open

SatelliteData interface should be optional #52

drekbour opened this issue Nov 24, 2019 · 5 comments

Comments

@drekbour
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@drekbour
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants