Null safety: Anti-pattern example Function to return indices matching a condition fun indicesIncludingText(list: List?, searchText: String?): Set? { What’s wrong with this code? if (searchText == null) { return null } return list.orEmpty().asSequence() .withIndex() .filter { (_, value) -> value.contains(searchText) } .map { (index, _) -> index } .toSet() }
Null safety: Solution, option 2 Write documentation for null argument class TextUiElement { fun setText(text: String?) { ... } } /** * Shows [text] in this UI element. * * This UI element is hidden with a null argument * while an empty string `""` is shown explicitly. */
Scope function: Anti-pattern example Cache layer for query/response class ResponseValueCache(...) { private val cachedValues: MutableMap = mutableMapOf() } What’s wrong with this code? fun getCacheOrQuery(queryParams: Q): V? = cachedValues[query] ?: queryClient.query(queryParams) .let { when (it) { is Response.Success -> it.value is Response.Error -> null } }?.also { cachedValues[query] = it }
Scope function: Problem Scope functions just for chaining .let { when (it) { is Response.Success -> it.value is Response.Error -> null } }?.also { cachedValues[query] = it }
Scope function: Problem Scope functions just for chaining
- Not easy to find the receiver
- The important side-effect is hidden .let { when (it) { is Response.Success -> it.value is Response.Error -> null } }?.also { cachedValues[query] = it }
Scope function: Problem Scope functions just for chaining
- Not easy to find the receiver
- The important side-effect is hidden .let { when (it) { is Response.Success -> it.value is Response.Error -> null } }?.also { cachedValues[query] = it }
Data class 1/2: Anti-pattern example A data class with
- A readonly property of a mutable object
- A non read-only property of an immutable object data class FooModel( val mutableElement: MutableElement, var immutableElement: ImmutableElement ) What’s wrong with this code?
Data class 2/2: Anti-pattern example A data class of UI layout attributes data class FooLayoutParameters( val titleText: String, val backgroundColorInt: Int, val buttonText: String, val onButtonClickListener: () -> Unit ) What’s wrong with this code?
Data class 2/2: Solution Confirm every property is data class friendly
If not...
- Use non-data class
- Split properties into two classes class FooLayoutAttributes( val titleText: String, val backgroundColorInt: Int, val buttonText: String, val onButtonClickListener: () -> Unit )
Data class: Other anti-patterns Non-product data model
Special case: Should be replaced with a sealed class // An invalid instance is easily created by `copy`. data class CoinState(val coinCount: Int, val coinText: String) { constructor(coinCount: Int): this(coinCount, "Coins: $coinCount") } data class Response(val result: R? = null, val error: E? = null)
Extension 1/3: Anti-pattern example Parses the receiver and creates a UserModel instance
- At the top-level of a Kotlin file fun String.toUserModel(): UserModel? { val userId = ... ?: return null val userName = ... ?: return null ... return UserModel(userId, userName, ...) } What’s wrong with this code?
Extension 2/3: Anti-pattern example An extension which modifies an instance member
open class FooClass { private val registeredElements: MutableSet = mutableSetOf() protected fun List.register() { registeredElements += this } } What’s wrong with this code?
Extension 2/3: Problem The implicit receiver hides modification register modifies the parent property class BarClass(...): FooClass() { fun someFunction() { ... someDataList.register() } }
- An instance method is enough open class FooClass { protected fun register(list: Collection) { registeredElements += list } } ... val listToRegister = dataList.map...filter... register(listToRegister) - A receiver-less function implies that this can be the receiver
- Observable instance is observed while ObserverScope is alive
interface Observable { /** * Calls `action` when this instance is updated and `scope` is alive... */ fun observe(scope: ObserverScope, action: (T) -> Unit) }
Extension 3/3: Anti-pattern example (cont.) An extension allows to pass ObserverScope as the receiver fun ObserverScope.observe(observable: Observable, action: (T) -> Unit) = observable.observe(this, action) What’s wrong with this code? class FooObserver(private val observerScope: ObserverScope) { init { ... observerScope.observe(observable) { /* do something */ } observerScope.observe(anotherObservable) { /* do another thing */ }
Extension 3/3: Problem Non obvious prioritization of function calls
- Example: A class inheriting both ObserverScope and Observable class BadClass: ObserverScope(), Observable { ... } fun function(badInstance1: BadClass, badInstance2: BadClass) { // As `Observable.observe` badInstance1.observe(badInstance2) { ... } // As `ObserverScope.observe` (badInstance1 as ObserverScope).observe(badInstance2) { ... }
1. Remove extensions flipping the receiver and the parameter
2. Give different names for such extensions fun ObserverScope.observe(observable: Observable) { ... } fun Observable.observedDuring(scope: ObserverScope) { ... }
Extension: Other anti-patterns With inheritance (cf., https://kotlinlang.org/docs/extensions.html#extensions-are-resolved-statically)
With down-casting for non-sealed class fun Base.getName() = "Base" fun Derived.getName() = "Derived" val base: Base = Derived() base.getName() // "Base" val RpcFrameworkException.errorCode: ErrorCode get() = when (this) { is FooException -> fooErrorCode is BarException -> barErrorCode else -> ErrorCode.UNKNOWN }
Default argument 1/2: Anti-pattern example An extension filtering even/odd numbers What’s wrong with this code? fun List.filterParity(takesEven: Boolean = false): List { val expectedRemainder = if (takesEven) 0 else 1 return filter { it % 2 == expectedRemainder } } - true: Returns even elements
Default argument 2/2: Anti-pattern example Changing the logic depending on which parameter is given fun setBackground(colorInt: Int? = null, themeType: ThemeType? = null) { when { colorInt != null -> ... themeType != null -> ... } } What’s wrong with this code? view.setBackground(colorInt = Color.WHITE) view.setBackground(themeType = ThemeType.PRIMARY)
Default argument 2/2: Problem The number of arguments might not be one
- The behavior is not predictable // This may apply to both arguments. // Or, one of the arguments takes the priority. view.setColor(colorInt = Color.WHITE, themeType = ThemeType.PRIMARY) // This may do nothing, or may throw an exception. view.setColor()
3. Define a sealed class of the parameter fun setBackground(model: BackgroundModel) { ... } sealed class BackgroundModel { class Color(val colorInt: Int) : BackgroundModel() class Theme(val themeType: ThemeType) : BackgroundModel() }
Sealed class 1/2: Anti-pattern example A sealed class Color with child data classes: Rgb, Cmy, and Hsl sealed class Color { data class Rgb(val red: UByte, val green: UByte, val blue: UByte) : Color() data class Cmy(val cyan: UByte, val magenta: UByte, val yellow: UByte) : Color() data class Hsl(val hue: AngleInt, val saturation: PercentInt, ...) : Color() } What’s wrong with this code?
Sealed class 1/2: Problem Ambiguity on equivalency val RED_1: Color = Color.Rgb(red = 255u, green = 0u, blue = 0u) val RED_2: Color = Color.Cmy(cyan = 0u, magenta = 255u, yellow = 255u) RED_1 == RED_2 // Can be false!
- Create constructors/factories to convert to the "main" unit data class Color(val red: UByte, val green: UByte, val blue: UByte) { companion object { fun fromCmy(cyan: UByte, magenta: UByte, yellow: UByte) : Color = Color((UByte.MAX_VALUE - cyan).toUByte(), ...) fun fromHsl(hue: AngleInt, ...) : Color = Color(...)
Sealed class 1/2: Note of solution 1 Resolution and value domain may differ
- Rounding error
- HSL color has "hue" even for black or white val redHue = AngleInt.of(0) val blackColor = Color.fromHsl( hue = redHue, saturation = PercentInt.MIN, lightness = PercentInt.MIN ) blackColor.hue != redHue // May be true
Sealed class 2/2: Anti-pattern example Data model of "comment" for a specific location
sealed class GeoLocationComment { } What's wrong with this code? class AtLatLon( val commentText: String, val latitude: Long, val longitude: Long ) : GeoLocationComment() class AtPlace( val commentText: String, val placeId: Long ) : GeoLocationComment()
- Required to take a common property val commentText = when(locationComment) { is GeoLocationComment.AtLatLon -> locationComment.commentText is GeoLocationComment.AtPlace -> locationComment.commentText }
Sealed class 2/2: Solution? Extract the common property to the parent
sealed class GeoLocationComment { class AtLatLon( val commentText: String, val latitude: Long, val longitude: Long ) : GeoLocationComment() class AtPlace( val commentText: String, val placeId: Long ) : GeoLocationComment() }
Sealed class 2/2: Solution? Extract the common property to the parent
sealed class GeoLocationComment { class AtLatLon( commentText: String, val latitude: Long, val longitude: Long ) : GeoLocationComment() class AtPlace( commentText: String, val placeId: Long ) : GeoLocationComment() }
Sealed class 2/2: Solution? Extract the common property to the parent
sealed class GeoLocationComment(val commentText: String) { class AtLatLon( commentText: String, val latitude: Long, val longitude: Long ) : GeoLocationComment(commentText) class AtPlace( commentText: String, val placeId: Long ) : GeoLocationComment(commentText) }
Sealed class 2/2: Solution? Extract the common property to the parent
sealed class GeoLocationComment(val commentText: String) { class AtLatLon( commentText: String, val latitude: Long, val longitude: Long ) : GeoLocationComment(commentText) class AtPlace( commentText: String, val placeId: Long ) : GeoLocationComment(commentText) }
Sealed class 2/2: Other problems 1. Need to update all the children to add a new property
2. open val is required to make a data class
sealed class GeoLocationComment( val commentText: String, val commentAuthorId: Long ) { class AtLatLon( commentText: String, commentAuthorId: Long, ... ) : GeoLocationComment(commentText, commentAuthorId) ...
Sealed class 2/2: Other problems 1. Need to update all the children to add a new property
2. open val is required to make a data class
sealed class GeoLocationComment( open val commentText: String, open val commentAuthorId: Long ) { data lass AtLatLon( override val commentText: String, override val commentAuthorId: Long, ... ) : GeoLocationComment(commentText, commentAuthorId) ...
Sealed class 2/2: Solution Minimize the sealed class responsibility
- Split the sealed class of location type from GeoLocationComment sealed class GeoLocation { class LatLon(val latitude: Long, val longitude: Long) : GeoLocation() class Place(val placeId: Long) : GeoLocation() } class GeoLocationComment( val commentText: String, val location: GeoLocation )
line_developers
makamaka
blue_goheimochi
takashiinui
yayoi_dd
uzulla
asazutaiga
tsubakimoto_s
daina0321
oracle4engineer
wadayusuke
aramaki
takufujii
bermonpainter
searls
jlugia
swwweet
mongodb
brianwarren
matthewcrist
shpigford
wjessup
jonrohan
erikaheidi
lauravandoore