こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。
この記事は、毎週木曜の定期連載 "Weekly Report" 共有の第 57 回です。 LINEヤフー社内には、高い開発生産性を維持するための Review Committee という活動があります。ここで集まった知見を、Weekly Report と称して毎週社内に共有しており、その一部を本ブログ上でも公開しています。（Weekly Report の詳細については、過去の記事一覧を参照してください）
百見は一 fetch にしかず
テキストメッセージを送受信する アプリケーションを実装しているとします。以下の sendMessage 関数では、引数として受け取った MessageModel に変換し、それをクエリパラメータとして送信する API を呼び出しています。
private fun sendMessage(messageModel: MessageModel): SendResult {
val sendRequestParams = toSendRequestParams(messageModel)
// ... snip...
return try {
val response = apiClient.sendMessage(sendRequestParams)
SendResult.Success(response)
} catch (e: ApiException) {
SendResult.Failure(e.toErrorType())
}
}ここで、次に示す diff のコードレビュー依頼を受けたとしましょう。この変更では「メッセージテキストが空文字列か空白のみのときは、INVALID_MESSAGE を返す」という早期リターンのコードが追加されています。
private fun sendMessage(messageModel: MessageModel): SendResult {
+ if (messageModel.text.isBlank()) {
+ return SendResult.Failure(MessageErrorType.INVALID_MESSAGE)
+ }
+
val sendRequestParams = toSendRequestParams(messageModel)
// ... snip...
return try {
val response = apiClient.sendMessage(sendRequestParams)
SendResult.Success(response)
} catch (e: ApiException) {
SendResult.Failure(e.toErrorType())
}
}このようなコードレビュー依頼を受けたときに、注意するべき点はありますか？
見えないものを見ようとする
「条件でフィルタする」というコードが追加された場合、同じような条件のフィルタが他の部分にないかを確認すべきでしょう。より一般的に言うと、コードの変更があった場合、似たようなコードが同じスコ ープや呼び出し元、呼び出し先にないか確認する べきと言えます。今回の例では、以下の可能性について確認すると良さそうです。
sendMessageの呼び出し元がすでにisBlankのケースを除外している。toSendRequestParamsがisBlankの場合に「無効な」パラメータオブジェクトを返す。(その無効なパラメータをapiClient.sendMessageに渡すとMessageErrorType.INVALID_MESSAGEが返る。)
また、呼び出し元/先でのフィルタの条件が、部分的に重複している場合もある点に注意するべきです。以下の toSendRequestParams は、空文字列の条件はフィルタしていますが、空白文字だけのケースはフィルタしていません。この場合、フィルタ条件の重複を見落としてしまうかも知れません。
fun toSendRequestParams(messageModel: MessageModel): SendRequestParams {
if (messageModel.text.isEmpty()) {
return SendRequestParams.INVALID
}
...
return ...
}Kotlin のような静的型付けの言語の場合、条件の重複を見落とさないようにするためにも、フィルタ済みの型とエラーの型を分けるとよいです。今回の例の場合は、SendRequestParams.INVALID のようなエラーを意味する null object を作るよりも、null を使うほうが明示的にエラーを区別できます。
private fun sendMessage(messageModel: MessageModel): SendResult {
val sendRequestParams = toSendRequestParams(messageModel)
?: return SendResult.Failure(MessageErrorType.INVALID_MESSAGE)
// ... snip...
return try {
val response = apiClient.sendMessage(sendRequestParams)
SendResult.Success(response)
} catch (e: ApiException) {
SendResult.Failure(e.toErrorType())
}
}
/** ... */
fun toSendRequestParams(messageModel: MessageModel): SendRequestParams? {
if (messageModel.text.isBlank()) {
return null
}
...
return ...
}見るための望遠鏡
コードレビューにおいて、よりよい提案をするには既存のコードと照らし合わせることが重要です。特に、以下のような部分を確認できるとよいでしょう。
- 変更したコードの呼び出し元と呼び出し先
- 変更したクラスの親/子クラスや兄弟クラス
- 変更した関数やクラスと似たような名前/役割/責任を持つ関数やクラス
これらのレビューを簡単にするには、そのコードの変更をチェックアウトし、IDE 上で確認するのがおすすめです。GitHub を使っているならば、以下の git コマンドでプルリクエストを fetch できます。
git fetch git@[git domain]:[organization]/[repository name].git refs/pull/[pull request number]/headgit remote が正しく設定されているならば、以下のように簡略化できます。
git fetch [remote] pull/[pull request number]/headさらに手軽に行うためには、以下のような、プルリクエスト番号からチェックアウトするスクリプト用意しておくよいでしょう。(ここでは、チェックアウト元の remote 名を origin と仮定しています。 remote 名に応じて適宜変更してください。)
#!/bin/bash -e
git fetch origin pull/$1/head; git switch --detach FETCH_HEAD一言まとめ
コードレビューをするときは、変更そのものだけでなく周辺の コードも確認する。
キーワード: code review, code change, diff checkout