こんな「リファクタリング」はいやだ

18169 ワード
Kotlin 新人プログラマ応援 リファクタリング Kotlin テキストリンク

この文章はもともとは、「新人プログラマ応援」タグ付きで掲載されていた外部サイトのシリーズ記事の一部が結構ひどい内容だったのでマサカリを投げてみようと思って書いたのですが、下書きのまま半年以上放置している間に元記事の在り処を忘れてしまい、今さら探すのも面倒なのでそのまま投稿するものです（追記：見付けたので各項の「元のコード」からリンクしました）。新人の誰かにとって本当の参考になるのを願いつつ。

リストの各要素に対する条件付き処理

元のコードは条件分岐が入れ子になっている。

orig.kt
fun execute(users: Sequence<User>) {
  for(user in users) {
    if (user.authenticated) {
      val connection = user.connection
      if (connection.enable) {
        val data = connection.data
        if (data.exists) {
          print(data)
        }
      }
    }
  }
}

インデントが深いのは嫌なので、if文の条件を反転させてガード節とすれば良いと思うかも知れない。

refactored.kt
fun execute(users: Sequence<User>) {
  for(user in users) {
    if (!user.authenticated) continue
    val connection = user.connection
    if (!connection.enable) continue
    val data = connection.data
    if (data.exists) {
      print(data)
    }
  }
}

言語によっては (例えばC, Pythonなどなら) これで良いかも知れない。

しかし、せっかくKotlinを使っているのなら、次のように書ける。

stream.kt
fun execute(users: Sequence<User>) {
  users.filter { user -> user.authenticated }
    .map{ user -> user.connection }
    .filter{ connection -> connection.enable }
    .map{ connection -> connection.data }
    .filter{ data -> data.exists }
    .forEach{ data -> print(data) }
}

リストに対して行いたい処理を宣言的に書けて明解になる。Java (8以降), Scala, Swiftなど、こういう書き方ができる言語を使っているのなら是非慣れておくべき。

ちなみにKotlinではitで略記するのが普通。

streamIt.kt
fun execute(users: Sequence<User>) {
  users.filter { it.authenticated }
    .map{ it.connection }
    .filter{ it.enable }
    .map{ it.data }
    .filter{ it.exists }
    .forEach{ print(it) }
}

論理演算子の置き換え？

orig.kt
fun check(mail: String, password: String) {
    if (mail.isNotEmpty() && password.isNotEmpty() && age.selected && terms.isChecked) {
        println("normalでの何らかの処理")
    }
}

元のコードを次のようにアレイとallで書き換えると読みやすくなる（？）

bad.kt
fun check(mail: String, password: String) {
    if (arrayOf(mail.isNotEmpty(), password.isNotEmpty(), age.selected, terms.isChecked).all{ it }) {
        println("badでの何らかの処理")
    }
}

……いや、これは明らかに駄目でしょう。A && B && ... では A が偽の場合に B 以降を評価しない（短絡評価）。それに対しアレイを生成する場合は全要素が評価されたうえで結果が格納されるので、本来不要な処理をやる場合が出てくる。

ストリームの中間操作では全要素が評価されるわけではない、というのは全然別の話。混同しないように。

条件分岐

元のコードのprintMessage関数は条件分岐に無駄があるので整理したい。

orig.kt
data class Person(val country: Country, val gender: Gender)

fun printMessage(p: Person) {
    if (p.country == Country.Japan) {
        println("日本の人")
    } else if (p.country == Country.England && p.gender == Gender.male) {
        println("イギリスの男の人")
    } else if (p.country == Country.England && p.gender == Gender.female) {
        println("イギリスの女の人")
    } else {
        println("その他の人")
    }
}

では、こう変えれば良いだろうか？

bad.kt
fun printMessage(p: Person) {
    if (p.country == Country.Japan) {
        println("日本の人")
    } else if (p.country == Country.England) {
        if (p.gender == Gender.male)
            println("イギリスの男の人")
        else
            println("イギリスの女の人")
    } else {
        println("その他の人")
    }
}

いや、せっかくKotlinを使うならwhen式を使ってほしい。

better.kt
fun printMessage(p: Person) {
    val msg = when (p.country) {
        Country.Japan -> "日本の人"
        Country.England -> when (p.gender) {
            Gender.male -> "イギリスの男の人"
            Gender.female -> "イギリスの女の人"
            else -> "その他の人"
        }
        else -> "その他の人"
    }
    println(msg)
}

それともう一点。ジェンダー判定を「男性でなければ女性」と勝手に解釈して書き換えるのは駄目でしょう。Genderにmale, femaleの2つしか（将来的にも）ないという確証はない。むしろ元のコードが一見冗長なのは、仕様として敢えてそうしてある、という可能性を考えないと。

最後に

拙速なリファクタリングは諸悪の根源。

Springboot統合vue(swagger 2 uiを使用)
phpのmysqliデータベース接続の2つの方法