Suggestion: Make ObjectMapper open

[jfbilodeau]

Greeting,

May I suggest we make ObjectMapper open? This way, we can create mappers for our games using inheritance:

class MyGameMapper: ObjectMapper() {
    init {
         registerType(....)
         // .....
     }
}

Thanks for the consideration!

[soywiz]

It is usually a good practice use composition over inheritance. Maybe you can try to do this:

fun MyGameMapper() = ObjectMapper().apply {
  registerType(...)
  // ...
}

You can then use it somewhere else the same way as if you had an object like MyGameMapper().

Does this work for you?

[jfbilodeau]

I agree with composition over inheritance, and right now, I'm using the same approach you are proposing down below. But it felt more cohesive to have a object representing my mappings than a function building my mappings.

[soywiz]

I strongly believe this case works better with composition, specially since we are not going to have open or abstract methods inside.

But maybe I'm missing some context; why do you feel it more cohesive with the object representing your mappings?

[jfbilodeau]

Oups! Looks like I answered on Discord instead of Github. Sorry. Here's my original response: I guess I'm arguing for RIAA (Resource Acquisition Is Initialization). I would feel better having an object fully initialized after instantiation than instantiate->configure->use. I recognize using a function to 'act' as a constructor offers the same behaviour syntactically but that feels awkward to me.

But I'm willing to accept that maybe I'm looking at the problem with too much of a C++/Java perspective and not enough of a Kotlin perspective. :stuck_out_tongue:

[soywiz]

I see your point. I believe we have several methods here to configure the mapper, so we are mutating the object in the end. But if I understand correctly what you want to achieve is to not be able to mutate the object later, to prevent potential misuses right? Thinking about how Kotlin resolves the issue in some APIs like buildList { ... }: buildList is a builder whose lambda receives a mutable list, while returns an immutable list. To avoid the cost of copying all the values to an immutable structure, what it does is to use interfaces. The mutablelist interface is used in the body, and a plain immutable list that is a subset of the mutablelist interface is returned. The object is still mutable, but the interface doesn't permit it.

Maybe here we can do something similar. For example an interface that has the methods from ObjectMapper, but the mutating ones (register*).

val result: IObjectMapper =  // the immutable one
objectMapper { // this: ObjectMapper  // the mutable one
   register...()
}

would that work for you?

[jfbilodeau]

Immutability was not necessarily what I had in mind--and I'm afraid this would add a layer of unnecessary complexity.

Now that I think about it, the ideal would be for mappings to be auto-generated. I don't think KorGE does that yet, or am I wrong?

[soywiz]

I see. Then im not fully sure about the benefits of fully constructing an object, over mutating it them providing an immutable version. I have not seen making classes open just to provide a custom constructor and not overriding members.

Another option could be something like:

class ObjectMapper(val init: ObjectMapper.() -> Unit = {}) { init { init() } }

that would happen at construction time with no need of extending the class.

Still you are right: This is not the way to go. I added support for kotlinx.serialization a few versions ago so you can add support for it with a single line im the build.gradle. First versions of korlibs were using jtransc for other targets and there we had reflection, so all the mappings were automatic. kotlinx.serialization was initially not available, later a bit immature and increases the output size substantially, snd thats mainly why the object mapper exists. But now might work properly. So you can try to serialize and deserialize using it instead of the ObjectMapper if it works fine for your case.

[jfbilodeau]

Ohhh! I like that:

class ObjectMapper(val init: ObjectMapper.() -> Unit = {}) {
init { init() }
}

As for kotlinx.serialization, I'll take a look. Thanks!

[soywiz]

I could add that one, but maybe I cant see the benefits of it over the apply, and why it is important to be initialized at construction. But well, not a big deal :) Let me know if kotlinx.serialization works for you 👍. I guess eventually I will deprecate the ObjectMapper.

[jfbilodeau]

Don't remove the ObjectMapper--Wishlair depends on it ;)

I'm of the opinion that when an object is instantiated, it should be ready to rock-and-roll. The construct->init->use (as opposed to construct->use) feels like an anti-pattern to me. Sorry if I'm being pedantic! ;)

[soywiz]

I’d say that RAII is specially important in languages like C/C++ and Rust, that have a pretty different memory model to the one proposed by the JVM, JS or Kotlin/Native.

If you think on that, when creating the mutable lists or maps inside this object, they are not initialized completely, but they are being mutable. And in the end, in C++ you also have a ::push_back method in arrays. ObjectMapper doesn’t have resources that need to be freed, and everything from there is going to be deallocated on GC.

In fact Korge was designed in a way where you didn’t have to handle yourself textures, but only bitmaps and textures are handled and GCed automatically.

So in general, except when opening files, everything is going to be deallocated via GC. And when doing that, there is a use method so you can do things like. openUse { } so the object lifecycle is well defined. The same goes for pools and other things.

I guess that if you encapsulate object creation and initialization in a single method, and somehow patternalized, and you don’t care about the exposed interface to be mutable, you shouldn’t be too worried. But I’m empathizing with you in that kind of concerns; sometimes I have to try too hard to sleep accepting all the technical debt this codebase has lol. Maybe you can relativize your thoughts on that by understanding the context, to not be that strict on that and to be able to sleep well tonight ;P

But well, let’s try to reach an agreement on this issue: I’m not going to make the class open, but I’m fine with providing an optional lambda parameter for the initialization. So if that works for you, let’s do just that, I’m already doing similar things in some classes. Also you can already have this functionality with a constructor-like global function like this one in your codebase:

// Please, don’t check the body implementation of this function if you are
having a bad day. Just asume it works, and OFC it is RAII!
Inline fun ObjectMapper(init: ObjectMapper.() -> Unit) =
ObjectMapper().apply(init)

Sorry for the joke comment, it is indeed not RAII-atomic-strictly but from the perspective of the caller it works like that. Hope it works for you :)