こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。
この記事は、毎週木曜の定期連載 “Weekly Report” 共有の第 25 回です。Weekly Report については、第 1 回の記事を参照してください。
TL;DR: つまり…どういうこと
ある開発者が以下のようなクラスを作成し、コードレビューを依頼したとします。
data class UserModel(
val userId: Int,
val accountName: String,
val ...
var onlineStatus: OnlineStatus,
var statusMessage: String
)これに対し、レビュアーは次のように コメントしました。
dataクラスにはcopy関数があります。プロパティの値を変更したい場合、そのプロパティを直接修正するのではなく、その関数を呼び出して別のインスタンスを作成することができます。
UserModel.onlineStatusとstatusMessageについて、copyを使わずvarで再代入可能にしているのは、これらの値が頻繁に更新されるのと、同一のオブジェクトを持つことで更新された状態を共有するためだと予想できます。しかし一般的には、可変なオブジェクトを共有すると、意図しないタイミングでプロパティが変わることによるバグを招きやすいです。状態の更新の度に新しいインスタンスを作成したほうが、頑健性を高く保ちやすいです。
結論として、このクラスのすべてのプロパティは
valにしたほうが良いと思います。インスタンスのコピーのコストが心配かもしれませんが、UserModelの状態が更新される頻度は高くないと思います。さらに、onlineStatusとstatusMessageだけが更新されるケースが多そうということから、それらは他の安定したプロパティから分離した方が良いかもしれません。例えば、以下のようにするのはどうでしょうか。data class UserModel( val userId: Int, val accountName: String, val ... ) class UserStatus( val userModel: UserModel, val onlineStatus: OnlineStatus, val statusMessage: String )
このレビューコメントはどう改善することができますか？
なんというか…ようするに…
レビ ューコメントでは、まず提案や要求を書き、理由はその後で補足するとよいでしょう。
この場合、改善されたコメントは次のようになります。
UserModelについて、値の更新頻度でクラスを分け、すべてのプロパティがvalになるようにしてください。class UserModel( val userId: Int, val accountName: String, val ... ) class UserStatus( val userModel: UserModel, val onlineStatus: OnlineStatus, val statusMessage: String )この変更は 2 つ考え方に基づいています。
1. オブジェクトの不変性: 可変なオブジェクトを共有するよりも、状態更新ごとに不変なオブジェクトを使い捨てた方が、意図しないタイミングでプロパティが変わることによるバグを防ぎやすいです。また、
data classでvarのプロパティを定義するとcopyとの使い分けで混乱を招くことがあるため、避けたほうが無難です。(詳細は https://… を参照)2. 値のライフサイクル:
onlineStatusとstatusMessageは他のプロパティよりも頻繁に更新されるようです。更新の頻度が高いプロパティと低いものを分けることで、誤った更新を防ぎやすくなります。
提案や要求を先に示すことで、レビュー依頼者はコメントの主要なポイントを最初に理解でき、その後で理由の妥当性を検証できます。こうすることで、読み返しの必要が少ない説明になります。
改善したコメントではさらに、「理由」の説明でも工夫しています。最初に項目がいくつあるかを提示し、各項目に見出しを付けることで構造を理解しや すくしているのです。
余談ですが、この weekly report では、重要な部分を意図的に最後に書くこともあります。これは、「素早い理解」よりも「考えてもらうこと」をより重視しているためです ;)
一言まとめ
レビューコメントでは、最初に提案や要求を書き、理由はその後で補足する。
キーワード: code review, review comment, explanation order