Map final
RobLoach opened this issue · 7 comments
I was looking to build upon tmx::Map by inheriting from it, and found that it is final. Could we remove the final keyword so I could inherit from it?
Hi,
There's no technical reason not to, as far as I remember. Have you tested it to see that it works in your use case? If so it should be no problem to remove.
just as an argument to keep Map as final:
Composition over inheritance
By allowing external projects to inherit Map the API of it is exposed into these projects and can not be easily changed without taking these projects into account.
Using an abstrat Interface and composition makes it easier to separate tmxlite from applications.
While I agree with that in principle (which is why it was made final in the first place), it should perhaps be considered that it is ultimately up to the user to decide how they utilise a library - C++ should be powerful enough to shoot yourself in the foot as Bjarne would say. Which is probably why I only have 3 toes, metaphorically speaking. Admittedly in this particular case the source could easily be modified by the user to suit their needs, without changing the library itself.
It might also be worth considering what final is actually for. Whilst it makes sense to prevent further inheritance in cases of polymorphism - for example the Layer class in tmxlite, and can help with devirtulisation optimisation, using final just to force others into a paradigm you personally prefer (particularly when that paradigm is still available without the final keyword) could perhaps be considered a misuse. I'll even admit that I probably use it far too much in this library, particularly in cases such as small data structs. Like all tools in a language, it has its uses, and should really only be applied as necessary. The hard part is, of course, determining what is deemed 'necessary' 🙂
Fair enough. I do not have a very strong opinion about it. In the end it is your decision and I can live with both :)
I'm able to hold the Map object in my own class structure, no problem. But building upon the existing one would ease the code a bit. There were a few things I was looking to override, like...
- Override file loading with
load(), to allow loading files with PhysFS instead - Add texture data for tilesets directly within the object itself
I'm fine if final is final™️ , as there are ways to manage the data elsewhere. You both have some great points about final's benefits, but not having final just would make a few things easier, despite inheriting from Map itself potentially not being the correct design pattern.
For now I shall probably leave it as final - although this topic has led me to have a hard think about just what it means, and it is possible I may address this again in the future.
Expanding the API to allow custom loading is a very interesting idea, which I may well act on as this would also help with platforms such as Android where fstream is not currently implemented by the NDK. The existing work-around is to load an entire map file into a string with whichever mechanisms are available and pass it to Map::loadFromString(). This is of course far from optimal and uses a fair amount of extra memory. A flexible, user-definable, interface would be ideal. Is there a particular design here which would be most useful in your case?
Adding custom data fields is probably a bit more complicated. The library was only ever meant to be a read-only representation of a tmx file, a bridge if you like, between parsing the file and using the data in whichever way a user's project requires. It ought to be possible to add something like a void pointer to each class, but I personally loathe that pattern (not to mention that it's more C than C++). std::any would perhaps fit the bill but that requires C++17, which I'm not ready to move the library to, just yet anyway. Again, if you have any design ideas which might fit I'd certainly consider them. My only real hard rules are that there should be no external dependencies which can't be included in the source, and that it shouldn't break backwards compatibility.
Thanks for the feedback!
Sounds good! I'll hold a Map instance instead of inheriting. Thanks for the design discussion! Learned more about final and where to use it 😉 👍