Title
a

Ayfri

01/11/2022, 6:51 PM
Is there a way to simplify this code or at least make it more beautiful ?
``````val List<Int>.minmaxDifference get() = maxOf { it } - minOf { it }
// chestRoom is a set, blockX/Y/Z are Int
// CHEST_ROOM_MIN_SIZE & MAX_SIZE are const val Int

if (chestRoom.map { it.blockX }.minmaxDifference in CHEST_ROOM_MIN_SIZE..CHEST_ROOM_MAX_SIZE) return false
if (chestRoom.map { it.blockY }.minmaxDifference in CHEST_ROOM_MIN_SIZE..CHEST_ROOM_MAX_SIZE) return false
if (chestRoom.map { it.blockZ }.minmaxDifference in CHEST_ROOM_MIN_SIZE..CHEST_ROOM_MAX_SIZE) return false``````
e

ephemient

01/11/2022, 7:05 PM
I would probably write one of
``````inline fun <T, R : Comparable<R>> Iterable<T>.minmaxOrNullBy(selector: (T) -> R): Pair<R, R>?
chestRoom.minmaxOrNullBy { it.blockX }?.let { (min, max) -> max - min in CHEST_ROOM_MIN_SIZE..CHEST_ROOM_MAX_SIZE }

inline fun <T> Iterable<T>.largestDeltaBy(selector: (T) -> Int): Int
chestRoom.largestDeltaBy { it.blockX } in CHEST_ROOM_MIN_SIZE..CHEST_ROOM_MAX_SIZE``````
r

Ruckus

01/11/2022, 7:05 PM
I'm not sure what your idea of beautiful is, but I think the biggest problem with the code is it's not immediately obvious what it's doing, and you have to reason it through to realize you're just doing a box bounds check. I would probably make a bounds object of some sort, and then define a
``fun Set<ChestRoom>.calculateBounds(): Bounds``
, and then
``return bounds.dx in MIN..MAX && bouds.dy in MIN..MAX && <http://bounds.dz|bounds.dz> in MIN..MAX``
👍 2
e

ephemient

01/11/2022, 7:06 PM
you could choose to be clever and write something like
``````arrayOf(::blockX, ::blockY, ::blockZ)
.any { largestDeltaBy(it) !in CHEST_ROOM_MIN_SIZE..CHEST_ROOM_MAX_SIZE }``````
but that doesn't really help clarity in this case, IMO
a

Ayfri

01/11/2022, 7:08 PM
okay I see, it's a bit more clearer at least
r

Ruckus

01/11/2022, 7:10 PM
But then my lack of understanding could just be my inexperience in the domain, so take what I say with the appropriate portioning of salt.
b

bezrukov

01/11/2022, 8:16 PM
I would consider switching from blockX/blockY/blockZ in the model to just an array, most likely you have similar repetitive code in other places In this case all the code will be simplified without a largestDeltaBy (if you still need I would suggest an
``amplitudeOf { key }``
name):
``````return (0 until AXES).none { axe ->
chest.maxOf { it.block[axe]} - chest.minOf { it.block[axe] } in  CHEST_ROOM_MIN_SIZE..CHEST_ROOM_MAX_SIZE
}``````
return (0 until AXES).