レビューするときにみんなどうしてる？
こんな会話があったので少し書いてみた。
個人的な主張ばかりなのでぜひコメントがあればお気軽にください。
## そもそもコードはどう合ってほしいか
私が価値があるソースコードだと思っているのは以下をみたすもの
- 仕様を満たす実装がされている
- 規約、アーキテクチャに沿っている
- 他者から見てわかりやすい
仕様を満たす実装がされている
超単純に計算ミスをしてくれるようなプログラムはなんの価値もない。
2つの数値を与えられて、その掛け算の結果を返す関数を作ってほしい
と言われたらエンジニアは 2つの数値を与えられて、その掛け算の結果を返す関数 を作るべきである。
Goで実装するなら
func product(x, y int) int {
return x * y
}
だろう。間違っても他の計算結果を返すことはあってはならない。
規約、アーキテクチャに沿っている
プロジェクトではある一定のルールで書かれていることがほとんどである。
例えば変数名はスネークケース、関数/メソッド名はキャメルケースとすることが約束されていたら従うべきだ。
func sampleFunc() {
var sample_variable = "sample variable"
fmt.Println(sample_variable)
}
であってほしい。
func sample_func() {
var sampleVariable = "sample variable"
fmt.Println(sampleVariable)
}
となってはならない。
他者から見てわかりやすい
他者から見てわかりやすいかの観点は様々あるだろう。
バイブルのリーダブルコードだったり、言語による方針だったり一言でまとめるのは不可能だと思う。
がそれだと主張のない記事になるので筆者の考える読みやすいコードを書き並べてみます。
- 不要な記述がされていない
- 各命名が適切
- 簡潔に書かれている
粒度が最適化されていないですが、このままかいてみます。
不要な記述がされていない
デバッグ関数だったり、テスト時に使用した変数などが神聖なデフォルトブランチにある状況はキレイとは言えないだろう。
このあたりはレビューで取り除くべきでLinterなどで対応可能なものは自動的に検知できるようにしておこう。
Goであればfmt.Println()は使わずにログ専用のパッケージを使うことや自作のセットを準備しよう。
各命名が適切
個人的には命名をとても大切にしていきたい。
コメントを書いてわかりやすくすればいいじゃんという考えもあるがこの後に紹介する簡潔さと基本トレードオフなので
コメントを書く必要がない状況を作るという考えを筆者は持っている。
（ちなみにこの考え方は尊敬する先輩エンジニアに教えてもらった。エンジニアとしての姿勢を最初に教えてもらい今でも大変感謝している）
サンプルを見てみよう。
整数の引数を受け取り偶数であればtrue, 奇数であればfalseを返す関数を考えてみる。
２つのコードを見比べてほしい。
func judgeEven(a int) bool {
// aが2で割り切れたらtrue、割り切れないならfalseを返す
return a % 2
}
func isEven(num int) bool {
return num % 2
}
処理内容は全く同じで関数名と引数の名前を変えてみた例ですが個人的には後者のコードを推したい。
理由としては関数名から返り値がboolでだと予測できること、引数名がより詳細で情報を持っていることが挙げられる。
（Goは静的型付けなのでこの行数ではそんなに恩恵は無いが、処理がより複雑になったときには聞いてくる。）
関数と変数だけでなくクラス、構造体、メンバー、プロパティのキー名などありとあらゆる命名は考え抜きたい。
簡潔に書かれている
これは言わずと知れたことであるが、改めて主張したい。
特に1ファイルが数千行あったり、1関数が100行を超えている場合は適度に切り出すべき。
これを切り出すときも適切にファイル名/関数名を命名してほしい。
レビューするとき
さてここまでで良いコードは何かの持論を述べてきたがレビュー観点も述べていこうと思う。
まず最初に言いたいのはレビューをしているときは時間が限られているということ。
最強のソースコードを組み上げるのに10年かかっていたらプロダクトのリリースなんて到底できない。
これが全てのレビューに付随してくるので、ある落とし所を探しておくと良いと思っている。
個人的には2つの軸で見ている
- マストで修正してほしいところ
- 時間に余裕があれば直してほしいところ
マストで修正してほしいところ
前述したこの2点は直すべきでApproveしてはならないと思っている。
- 仕様を満たす実装がされている
- 規約、アーキテクチャに沿っている
仕様を満たす実装ができていなければコードとして意味がないし、規約の違反を一度でも許すと規約の意味がなくなる。
ここは妥協してはいけないと思っている。
時間に余裕があれば直してほしいところ
一方で自分ならこうするがレビュイーが別の書き方をした場合もある。
マストで修正すべきではない場合は個人的には時間見合いでコメントを出すかを決めている
特に命名でfetchのほうがふさわしくて使ってほしいけどgetと書かれている場合には動作には一切影響しない。
一方で命名を最適化し続けると可読性が上がり後にコードを見たときの生産性が上がってくる恩恵がある。
なので時間が許されるのであれば後に恩恵がある部分はこだわって直していきたい。
まとめ
時間が限られた中で成果を上げる必要があるが、一定の品質を担保するという難しい部分を任されていると思う。
筆者のスタイルが参考になれば嬉しいです。