<https://kotlinlang.slack.com/archives/C0B8MA7FA/p...

# codereview

[Matyáš Vítek]

https://kotlinlang.slack.com/archives/C0B8MA7FA/p1731254297620149

[Joffrey]

I really dislike using the elvis operator for a mutating operation, especially with so much code there. But there are also several things we could improve on the second approach: we could extract the key lookup to a variable before checking, or even better, use getOrPut and then consistently add to the overrides list.

[Joffrey]

(also, please share code with actual code blocks instead of screenshots, otherwise it's very inconvenient for people to copy your code and make adjustments to make suggestions)

[Joffrey]

That would be better IMO:

val item = itemModels.getOrPut(parentPath) {
    ItemModel(
        key = NamespacedKey.minecraft("items/generated"),
        layers = mapOf("layer0" to NamespacedKey.minecraft("item/$parentMaterial")),
        overrides = mutableListOf(), // probably make this a default value in the constructor
    )
}
item.overrides += modelOverride

[Joffrey]

Or even better, extract the construction of generated items to a function:

val item = itemModels.getOrPut(parentPath) { generatedItem(material = parentMaterial) }
item.overrides += modelOverride

// later
fun generatedItem(material: String) = ItemModel(
    key = NamespacedKey.minecraft("items/generated"),
    layers = mapOf("layer0" to NamespacedKey.minecraft("item/$material")),
    overrides = mutableListOf(), // probably make this a default value in the constructor
)

[Matyáš Vítek]

Thanks for the suggestions and sorry for the images. I only use those because Slack doesn't support proper syntax highlighting

[Daniel Pitts]

@Joffrey I was going to make a similar suggestion on the getOrPut. That seems like the best approach.

[Daniel Pitts]

@Matyáš Vítek I think you can get code highlighting if you use /snippet...

[Daniel Pitts]

Untitled.kt

Untitled.kt

[andries.fc]

2nd one, as it is more readable.

[Klitos Kyriacou]

Joffrey's suggestion is not only more readable in my opinion than either of the two options, but it also fixes a bug in the second snippet. Multithreaded code may throw an NPE at itemModels[parentPath]!! because it may be deleted by another thread between checking and using.

[Joffrey]

@Klitos Kyriacou I haven't re-checked right now (I'm on my phone), but AFAIR getOrPut is not atomic because it's implemented as an extension, so it most likely also contains a race condition

[Klitos Kyriacou]

@Joffrey you're right, it's not guaranteed to be atomic, but at least it doesn't suffer from possible NPE like the second snippet does, as it does a lookup only once. There is also an extension function on ConcurrentMap which is atomic.

[Matyáš Vítek]

Race conditions wouldn't be a problem in my case, it won't be multithreaded