こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。
この記事は、毎週木曜の定期連載 "Weekly Report" 共有の第 35 回です。 LINEヤフー社内には、高い開発生産性を維持するための Review Committee という活動があります。ここで集まった知見を、Weekly Report と称して毎週社内に共有しており、その一部を本ブログ上でも公開しています。（Weekly Report の詳細については、過去の記事一覧を参照してください）
引数の持ち腐れ
ある UI 要素の背景を動的に変更することを想定しましょう。以下の setShowcaseBackground 関数は、背景の色や画像を示す backgroundType を引数に取ります。BackgroundType.IMAGE が与えられた場合、もう一つの引数 imageUrl で指定された画像を背景として設定します。
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
}
}
}このコードに問題点はなにかありますか？
すべての引数を使い切る
このコードの問題は、一部の引数が、条件によっては使われない ことです。imageUrl の値は backgroundType が IMAGE のときだけ有効で、それ以外では意味を成しません。これは、以下のような誤解を招くコードの原因になります。
setShowcaseBackground(BackgroundType.MIDNIGHT, iconImageUrl)このコードから予想される動作は、「iconImageUrl に MIDNIGHT のカラーフィルタをかけて表示する」や「MIDNIGHT の単色背景の上に iconImageUrl を表示する」というものです。しかし実際には iconImageUrl が無視されるという、コードを読んだ開発者の予想外の動作になってしまいます。
更に、ERROR_IMAGE のドキュメンテーションが実際の挙動と異なるのも問題です。ERROR_IMAGE は fetchImage が失敗したときに表示される画像と説明されているのですが、実際には実引数の組み合わせが不正な (backgroundType が IMAGE で imageUrl が null) ときも表示されます。
この問題を解決する方法としては、以下の 2 つが挙げられます。
- 関数を分割する
- 引数の型を再構成する
Option 1: 関数を分割する
条件ごとに関数を分けることで、不要な引数を削除できます。以下のコードでは、色と画像で関数を分けています。
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
}関数を分けた結果、共通するロジックがコピーされてしまった場合は、以下のような改善できます。
- 共通部分を private 関数などで抽出する
- 一方の関数がもう一方を呼び出すように変更する
この「関数を分割する」方法は、特に「どちらの条件になるかが、呼び出し元によって静的に決まる」場合に有効です。逆を言えば、条件が動的に決まる状況で関数を分割すると、その条件分岐のコードが関数の呼び出し元側でコピーされてしまう可能性があります。その場合は、次の「引数の型を再構成する」手段を試みてください。
Option 2: 引数の型を再構成する
条件によって持ちうる値が異なる場合、条件ごとに型を分け、その直和型を引数として取る方法もあります。以下のコードでは、色と画像で型を分け、その直和 BackgroundType を引数として与えています。こうすることで、色を指定しつつ 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)ただし、直和型は時として強力すぎる場合もある点に注意してください。呼び出し先で型を区別する必要がない場合は、1つの型にまとめられるケースもあります。BackgroundType の例では、画像の取得を呼び出し元で実行して良いならば、以下のように一つのクラスにまとめられます。
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 =
...
}
}一言まとめ
一部の条件でのみ使われる引数を作らないようにする。
キーワード: argument, orthogonal relationship, sum type