はじめに

タブやスペースの数（インデント）とか変数名のセンスだけを指摘してコードレビューしたぜ！となっていいのは小学生までと言われています^[要出典]。このエントリでは主にScalaをターゲットにして、どういうところに気をつけると効果的にレビューできるのか？について僕の個人的な経験に基づきつつ紹介しようと思います。

こういうコードがでてきたら、これは慎重に見るべきポイントだな！というのを紹介していきます。コードの全てのを見ていくのは非常に大変なので、こういう気になるところだけ見るような形にしています。また、このエントリではサンプルコードとしてScalaを利用してます。

型Aをとって型Bを返す関数

こう言われるとやや謎かもしれないですが、たとえば次のような関数です。

def toInt(str: String): Int = ??? // 実装があるとする

こういう関数があったら、僕は脳死で次のことは必ず考えます¹⁾脳死で考えるって無理だという指摘がありましたが、そこそこのレベルではこういうのを脊髄で考えられるようになります。任意の非チューリング完全な計算は脊髄で行なえることが知られていません^[要出典]。。

• このような変換において例外が生じることはないか？

冷静に考えてみれば、A => Bみたいな関数fが例外を出すことなく動作するためには、fは全域関数でないといけないです。そこで例のtoIntですが、自然な意味を考えればあきらかにこれは全域的には振舞えないでしょう。たとえば文字列の"100"を100にすることはできますが、"hoge"を数字にするのは自然な定義はないからです。自然に変換できないものは-1にする、みたいな実装も可能です。ですが、つまりそれは自明ではない実装になるのでレビューするべき対象になります。
また、Scalaに限った話で言えば、この手の変換では次のようなこともチェックに値します。

• Javaの関数を呼び出していないか？

なぜかというとJavaの関数は高確率で例外を送出するので、そのケアができていないと例外が飛んでしまいます。なので、Javaの関数を呼び出すようなScalaのコードは適切にtry-catchするかEitherやTryのような型になるのが自然です。

型F[A]をとって型F[B]を返す関数

これもしばしばやってしまうコードです。例で言うと次のようなコードです。

def isEvenOpt(opt: Option[Int]): Option[Boolean] = ???

これは高階な型FとしてOptionを使った例となります。こういう関数は、まあたぶん動作はするんですが正直汎用性があまりないと言わざるを得ないです。なぜかというと、Scalaで取り扱うような高階な型（たとえばSeq, Option, Either, Future）は大抵はモナドやファンクターのような抽象的な構造として扱えます。従って、このようなOptionでしか使えないような具体的すぎる実装を作るよりも、次のような関数を作ればよいです。

def isEven(i: Int): Boolean = ??? // こいつを実装する
def isEvenOpt(opt: Option[Int]): Option[Boolean] = opt.map(isEven) // こっちは簡単に得られる

モナドやファンクターといった構造は、単なるA => Bのような関数をモナドなどの世界に連れていくのは得意なので、そういう関数を作ってmapやflatMapなどで持ち上げるのが定石です。ただ、isEvenのような構造を見ると最初に指摘した問題として、この関数が全域じゃないとNGでは？になってしまいます。ただ、ここでモナドの便利な性質としてflatMapを使って次のようにします。

def isEvenOpt2(i: Int): Option[Boolean] = ???

こうすると、for式の中で非常に使いやすくなります。

for {
  a <- optionWoTsukuruNanikaFunction1
  b <- optionWoTsukuruNanikaFunction2
  c <- isEven(a + b)
} yields ???

こんな感じで、for式の中で束縛する型がモナドの内側の型なので、引数は単なるAのような型にしておいて、一方で返り値はモナドに包まれた型にするという戦略が有効です。逆に引数がモナドにつつまれていたりする場合は、もちろんそうせざるを得ないときもあるものの、なにかが変な場合があるので実装を細かくレビューします。

map(???).flattenを使って構造を圧縮している

このコードはflatMapでほとんどは書き直すことができます。mapがモナドによって提供されている前提となりますが、今任意のモナドMがあるときに、型M[M[A]]となる値mに対して次を実行するとわかります。

val m: M[M[A]] = ???
m.flatMap(x => x): M[A]

ひとまず、mapとflattenのコストをそれぞれ支払うよりもflatMapの方がコストが軽いと思われますし、ソースコードの量も減るのでこちらがおすすめです。

逆に書き直せない場合というのは、Seq[Option[A]]といった構造をSeq[A]にしているといった時です。Optionは長さが0また1のSeqであるとみなせますから、Scalaではこのような型についてflattenができるようになっています。したがってmap(???).flattenをみつけたときは、二重になっているコレクションの型がそれぞれ違うのではないか？ということも考えるようにしています。

ソートしているコード

sortByとかなんでもいいんですが、そういう何かしらの方法を使ってソートしている部分は必ずレビューしています。ソートの方法が適切かどうか？という部分は本当にミスが生じやすいという経験があります。あまりある例ではないですが、頻繁にインサートしてソート、みたいな構造の場合はリストのような構造ではなくツリーにする、といったデータ構造の検討も視野に入れます。

リバースしているコード

あったらチェックします。まず、リバースしているということは、すでにそのリスト的な構造がなんらかの順序で並べられているわけですが、それがきちん並べてあるか？というところをチェックします。ほとんどないですが、ごくたまに遅延なコレクションをリバースしていることもあるので、そういった場合に危険がないかを見るようにしています。

引数なしString#getBytes

引数なしのString#getBytesは絶対ダメです。引数としてStandardCharsets.UTF_8など文字コードを必ず渡しましょう。与えていない場合はデプロイされたサーバーのデフォルト設定の文字コードなどが利用され、思わぬバイナリに変換される恐れがあります。

case e: Throwable =>で例外処理

　
これはFuture#recoverなどでやってしまいがちですが、Throwableというのはあらゆる例外を含んでいます。Javaでは基本的に「不必要に例外をキャッチしない方がいい」というスタンスです。つまりその関数でキャッチできるものだけをキャッチして、それ以外はそのままより上位の呼び出しに任せる、ということです。ですがThrowableでキャッチしてしまうと、スタックがあふれたとかヒープがなくなったみたいなもうこれ以上動作を続けると変なことになって、より深刻に壊れる状態もキャッチされてしまいます。キャッチされなければJVMがクラッシュしてLBから切り離されることで軽微な障害だったのに、壊れたJVMのままつっぱしった結果めちゃくちゃな障害になってしまったら目もあてられません。したがってこのような、なにもかもキャッチするコードは書く必要がないです。

過去に見たことがあるようなコードがある

これはコピペを疑うべきです。コピペをするなら、抽象クラスとか別のクラスとかそういうところで実装を共通化すれば、テストもそこに対してやればよいなど色々な工数が浮きます。コピペっぽいコードは、たとえコピペでなくても作者が抽象化できることに気がつかずやってしまう、ということもあるのでドンドン指摘していきましょう。基本的に1行コードが増えるたびに借金が1円増えている^[要出典]ので、あんまり増えないようにしたいですね。

入れ子になりすぎたモナドがある

たとえばOption[Seq[Int]]とかです。これはつまり、次の3パターンがあります。

1. None
2. Some(Nil)
3. Some(Seq(???))

これら1つ1つに意味がきちんとあるか？ということは考えなければなりませんし、ぱっと分からなければレビューコメントで意味を説明してもらう必要があります。モナドが重なれば重なるほどパターンが多くなっていって、それぞれの意味を全部考えなければならなくなります。また、これ意味ないな！となったということはあるモナドは実は必要がない、ということも考えられます。とにかくモナドを重ねるときは、全パターンについて考えてみてください。

モナドへのパターンマッチ

モナドでは、ときどきパターンマッチの必要があります。SeqとかOptionではパターンマッチせざるを得ないケースがもちろん存在しています。ところが、いくつかの場合これらはモナドの機能であるmapやflatMapといった抽象的な関数で解決できることが多いと思います。パターンマッチは基本的にそのモナドでしか動作しないので、かなり具体的な操作と言えます。ところがmapなどは他のモナドでも使えるので汎用性が高い抽象的なコードになり、再利用がより容易になります。

また、パターンマッチのcaseでcase ??? if ???のようなガードは基本的には使うべきではないです。これを利用するとパターンマッチの網羅性検査が死んでしまうので、使わなくて済む場合はなるべく避けた方がいいと思います。基本的にこのガードはcase ??? => if ??? else ???で置き換え可能なので、僕はガードがあるからといってレビューで落としはしませんが、僕自身のコードではガードを使うことはありません。

ファイルなどのリソースを操作している

例外が発生したとしても、ファイルやHTTPコネクションといったリソースは必ずクローズする必要があります。そうしなければリソースが掴みぱなしになって、ファイルディスクリプタがなくなったりといった深刻な問題になります。ファイルを開いているコードがある場合は、プロセスが落ちるような例外を除いたほとんどの例外が生じてもきちんとクローズできるようになっているか？をレビューしています。

謎の初期値やgetOrElse("")とか

なにかの都合でOptionといった構造を剥したいときに使ってしまいがちです。getOrElse(1)とかそういうのを使う場合はコメントが必要です。あるいはinitialUserStateとかなんかそういう初期値っぽい定数を定義したりするべきであって、0とか""みたいなありふれたリテラルを初期値に入れておくことは混乱の元になりがちです。ですから、初期値やgetOrElseなどをコメントなどなく使っている部分はたいていチェックしています。

余談ですがC言語の開発においてはNULL以外の数値は全て定数か何かとして定義しないと使わせてもらえなかった、という界隈の話を聞いたことがあります。さすがにそれはやりすぎと思いますが、とはいえ初期値はほとんどに名前が付けられるので、何か名前を付けておいた方がいいと思います。

まとめ

コードレビューは一瞬で上手くなるとは思わないのですが、こつこつやっていきましょう。