“now” looks better for me

# codingconventions

[enleur]

“now” looks better for me

[waqas]

out of 8 developers i asked you are the first one simple smile

[Czar]

I second @enleur's opinion.

[waqas]

What about reverse Christmas tree like brackets at the end?

[Czar]

I'll elaborate, "now" is better than before, but on its own I don't like it either.

[waqas]

@Czar I will extract creation of AssetBeacon one line before to see if it makes it any better, or do u have any other idea to make it better?

[enleur]

secondary constructor in AssetBeacon that accepts whole scannedBeacon

[enleur]

AssetBeacon(it, scannedBeacon)

[waqas]

AssetBeacon is one line data class, idk if it's worth it do that just for this one line 🤔

Anyhow the original problem is not there if extract AssetBeacon one line before instead.

when {
            !beaconsSeen.contains(scannedBeacon) -> {
                beaconsSeen.add(scannedBeacon)
                assetRepository.findIdByUuid(scannedBeacon.uuid)?.let {
                    logger.debug(tag, "sending asset beacon enter for $scannedBeacon")
                    val beacon = AssetBeacon(it, scannedBeacon.uuid, scannedBeacon.major, scannedBeacon.minor)
                    <http://bus.post|bus.post>(AssetEvent.AssetBeaconEnter(beacon))
                }
            }
        }

[Czar]

these are two better (strictly in my opinion) ways of writing the same thing without modifying AssetEvent implementation:

// 1
when {
	!beaconsSeen.contains(scannedBeacon) -> {
		beaconsSeen.add(scannedBeacon)
		assetRepository.findIdByUuid(scannedBeacon.uuid)?.let {
			if (logger.debugEnabled()) {
				logger.debug(tag, "sending asset beacon enter for $scannedBeacon")
			}
			AssetBeacon(
				it,
				scannedBeacon.uuid,
				scannedBeacon.major,
				scannedBeacon.minor
			)
		}?.run { <http://bus.post|bus.post>(AssetEvent.AssetBeaconEnter(this)) }
	}
}

// 2
when (scannedBeacon){
	!in beaconsSeen -> {
		beaconsSeen.add(scannedBeacon)
		assetRepository.findIdByUuid(scannedBeacon.uuid)?.let {
			if (logger.debugEnabled()) {
				logger.debug(tag, "sending asset beacon enter for $scannedBeacon")
			}
			AssetBeacon(
				it,
				scannedBeacon.uuid,
				scannedBeacon.major,
				scannedBeacon.minor
			)
		}?.run { <http://bus.post|bus.post>(AssetEvent.AssetBeaconEnter(this)) }
	}
}

[waqas]

thanks @Czar definitely looks better. I will go with #2. Also btw logger.debug takes care of .debugEnabled() check etc so that they are not everywhere else in code.

[Czar]

how? "sending asset beacon enter for $scannedBeacon" <- this will be interpolated regardless of how debug is implemented.

[waqas]

yes that's true. Was more concerned about having that If every where in code. Should pass as params, and format it latter in logger implementation.

[waqas]

#2 still has this though 😔

AssetBeacon(
                it,
                scannedBeacon.uuid,
                scannedBeacon.major,
                scannedBeacon.minor
            )

[Czar]

you can do better, if your logger is something custom, you can refactor it like this:```
@Suppress("NOTHING_TO_INLINE")
internal inline fun (() -> Any?).toStringSafe(): String {
try {
return invoke().toString()
} catch (e: Exception) {
return "Log message invocation failed: $e"
}
}

class YourLoggerClass {
/**
* Lazy add a log message if isDebugEnabled is true
*/
fun debug(tag: String, msg: () -> Any?) {
//I do not know what you do with tag, so this is just for illustration purposes
if (isDebugEnabled) debug("$tag: ${msg.toStringSafe()}")
}
}```
which I totally stole from MicroUtils KLogging 🙂
Then your logging will look like this and will be fully lazy: logger.debug(tag) { "your interpolated String with some logic, maybe" }

[Czar]

#2 still has this though

well, I'd go with @enleur's suggestion here and add the secondary constructor. It is a small tradeoff to reduce code complexity at the call site.

[waqas]

Good to know about KLogging solution, we opted for vararg instead, as we provide an interface for apps to plug their own logger implementations. I will try Klooger approach if it makes sense for our default logger simple smile ! thanks though 👏

[Czar]

Welcome 🙂