libktx / ktx

Kotlin extensions for the libGDX game framework
https://libktx.github.io/
Creative Commons Zero v1.0 Universal
1.35k stars 72 forks source link

Static instances removal #42

Closed czyzby closed 7 years ago

czyzby commented 7 years ago


Most static variables are pretty much justified and optional to use. For example, you can still benefit from gdx-assets utilities without the global AssetManager, but most applications will want to use only a single one anyway.

I don't think the global Skin variable should be removed from ktx-scene2d though, as it greatly simplifies and improves readability of UI building methods.

@kotcrab @sreich @MrPlow442 I'd love to hear your thoughts on this.

MrPlow442 commented 7 years ago

I believe I might be the least qualified here to suggest anything but here goes. I come primarily from web development where globals are frowned upon and are mostly replaced by singletons managed by DI frameworks. With that mindset I'd suggest to forgo globals where possible and let users decide where, how and when to instantiate and use these utilities. Is there any significant difference if you offer a global AssetManager or if you register it in ktx-inject or kodein or any other DI framework as a singleton? In both cases it should be equally trivial to reach the instance, with the DI giving additional benefits in TDD. I haven't yet used scene2d extensions so I'm afraid I cannot comment on that.

czyzby commented 7 years ago

@MrPlow442 That's basically my background and mindset, really. I originally transfered some of my personal Kotlin utilities without giving it much thought and I believe keeping the globals around was a mistake. I think deprecating all static instances (and utility functions that depend on them) makes sense, I'll eventually remove them in a future release.

kotcrab commented 7 years ago

I agree with removing static instances of I18N bundle and assets manager. Those can be handled better.

What about deprecating static context for ktx-inject, this would require passing context instance to every class that wants to use DI, right?

Scene2d.ui Skin makes me sad that it's not handled by the Stage itself, I think it's just better to keep it static here.

czyzby commented 7 years ago

What about deprecating static context for ktx-inject, this would require passing context instance to every class that wants to use DI, right?

This would require you to design your classes in such a way that you don't need it - injecting only the providers that you plan on using dynamically.

Scene2d.ui Skin makes me sad that it's not handled by the Stage itself, I think it's just better to keep it static here.

Agreed. Having to pass skin variable to every UI building methods would clutter the API and would be inconsistent with VisUI which provides Skin instance internally.