The original article was published on July 25, 2024.
Hello, I'm Munetoshi Ishikawa, a mobile client developer for the LINE messaging app.
This article is the latest installment of our weekly series "Improving code quality". For more information about the Weekly Report, please see the first article.
A phantom argument
Let's assume you want to dynamically change the background of a UI element. The following setShowcaseBackground function takes backgroundType as an argument, which indicates the color or image of the background. If BackgroundType.IMAGE is given, another argument imageUrl specifies the image to be set as the background.
enum class BackgroundType { MIDNIGHT, SKY_BLUE, MINT, PEARL, IMAGE }
class ShowcaseLayout(...) {
private val showcase: Showcase = ...
fun setShowcaseBackground(backgroundType: BackgroundType, imageUrl: Url?) {
val colorInt = backgroundType.colorInt
showcase.backgroundImage = colorInt?.let(Image::ofColor)
?: imageUrl?.let(::fetchImage)
?: ERROR_IMAGE
}
private fun fetchImage(imageUrl: Url): Image? { ... }
companion object {
/** An image displayed when image fetching fails. */
private val ERROR_IMAGE: Image = ...
private val BackgroundType.colorInt: Int?
get() = when (this) {
BackgroundType.MIDNIGHT -> ...
BackgroundType.SKY_BLUE -> ...
BackgroundType.MINT -> ...
BackgroundType.PEARL -> ...
BackgroundType.IMAGE -> null
}
}
}Is there a problem with this code?
Use all arguments
The problem with this code is that some arguments are not used under certain conditions. The value of imageUrl is only valid when backgroundType is IMAGE, and it is meaningless otherwise. This can lead to misleading code like the following:
setShowcaseBackground(BackgroundType.MIDNIGHT, iconImageUrl)The expected behavior from this code might be "display iconImageUrl with a MIDNIGHT color filter" or "display iconImageUrl on a solid MIDNIGHT background". However, in reality, iconImageUrl is ignored, leading to unexpected behavior for developers reading the code.
Additionally, the documentation for ERROR_IMAGE does not match its actual behavior. ERROR_IMAGE is described as the image displayed when fetchImage fails, but it is also displayed when the argument combination is invalid (when backgroundType is IMAGE and imageUrl is null).
There are two ways to solve this problem:
- Split the function
- Reconstruct the argument types
Option 1: Split the function
By splitting the function based on conditions, you can remove unnecessary arguments. The following code separates the functions for color and image.
enum class BackgroundColorType { MIDNIGHT, SKY_BLUE, MINT, PEARL }
...
fun setShowcaseBackgroundColor(colorType: BackgroundColorType) {
showcase.backgroundImage = Image.ofColor(colorType.colorInt)
}
fun setShowcaseBackgroundImage(imageUrl: Url) {
showcase.backgroundImage = fetchImage(imageUrl) ?: ERROR_IMAGE
}If splitting the function results in duplicated logic, you can improve it as follows:
- Extract common parts into private functions
- Modify one function to call the other
This "split the function" method is particularly effective when "which condition applies is statically determined by the caller". Conversely, if the condition is dynamically determined, splitting the function may result in duplicated condition-checking code on the caller side. In such cases, try the next method, "reconstruct the argument types".
Option 2: Reconstruct the argument types
If the values that can be held differ depending on the condition, you can separate the types for each condition and take their sum type as an argument. In the following code, the types are separated for color and image, and the sum BackgroundType is given as an argument. This makes it impossible to specify a color while passing imageUrl.
sealed interface BackgroundType {
class SingleColor(val colorType: ColorType): BackgroundType
class Image(val imageUrl: Url): BackgroundType
enum class ColorType { MIDNIGHT, SKY_BLUE, MINT, PEARL }
}
...
fun setShowcaseBackground(backgroundType: BackgroundType)However, note that sum types can sometimes be too powerful. If there is no need to distinguish types at the call site, they can be combined into a single type. In the example of BackgroundType, if image retrieval can be performed at the caller, it can be combined into a single class as follows:
class ShowcaseBackground private constructor(val image: Image) {
companion object {
val MIDNIGHT = fromColor(...)
val SKY_BLUE = fromColor(...)
val MINT = fromColor(...)
val PEARL = fromColor(...)
private val ERROR_IMAGE: Image = ...
fun fromImageUrl(url: Url): ShowcaseBackground {
val image = fetchImage(url) ?: ERROR_IMAGE
return ShowcaseBackground(image)
}
private fun fromColor(colorInt: Int): ShowcaseBackground =
...
}
}In a nutshell
Avoid creating arguments that are only used under certain conditions.
Keywords: argument, orthogonal relationship, sum type