レビューするときにみんなどうしてる？

こんな会話があったので少し書いてみた。

個人的な主張ばかりなのでぜひコメントがあればお気軽にください。

##　そもそもコードはどう合ってほしいか

私が価値があるソースコードだと思っているのは以下をみたすもの

仕様を満たす実装がされている
規約、アーキテクチャに沿っている
他者から見てわかりやすい

仕様を満たす実装がされている

超単純に計算ミスをしてくれるようなプログラムはなんの価値もない。

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行を超えている場合は適度に切り出すべき。

これを切り出すときも適切にファイル名/関数名を命名してほしい。