コードレビュー10年目がレビュアーとして、みてる部分をまとめてみた。

なにをもって、コードがきれい汚いと判断してきたかを言語化してみた。
レビュアの一つの参考になって貰えればいいかもしれない。

そのうち、鍛錬つんでたら、レビューイのさぼったリングが見えるようになる。

こういう部分はどうなってるのとか質問あれば、コメントいただけると嬉しいです。

コードの綺麗さ哲学

誰でも意図が理解できるコードは美しい
書いてないコードは美しい

vs 新記法

新記法をつかうと、大体ついてこれないメンバーがいくにんかでることがある。
新記法は、きれいではないは論理的にはとおらない。が、誰でもは理解できないコードになってしまった。

しかし、記法が理解できないだけで、意図は理解できればそれでいいと思ってる。

あと、短いコードになるなら積極的に採用したい。

ファイル

配置場所は適切か？

MVCやMVP、MVVM、ヘキサゴナル、クリーン、レイヤードなどのソフトウェアアーキテクチャがあるとおもう。
ぶっちゃけそのプロジェクトで何を使ってるかは知らないけども、おそらくテックリードはいるんだろう。

そのテックリードの意図にあった場所に配置されているか？

が綺麗さの焦点となる。

ここで一般論をださないのは、プロジェクトのファイル配置はテックリードが法だと思っているから、
悪法もまた法なり…

もちろん、テックリードが一般論でやるというのであればそれもよし。

個人的には、ベストプラクティスを探して使うのがいいとは思っている。

ファイル名（パス）と中身が一致？

まぁ、書いてるうちに、クラスをポンポン作ってしまうケースがあるだろう
もちろん、実装作業中のあなたに文句をいう筋合いはない。

ただ、ファイル名にそぐわないクラスが出来上がったときは、extract classしたほうがいいとおもう。

ファイル名の単一責務

1ファイル1モジュール（言語によってはパッケージ）ぐらいのテンションがいい。
1ファイル1クラスはたまにしんどいので、1小機能ぐらいのテンションでいたい。

並列してるファイルとの粒度の差

db
├─ user.py
└─ s3.py

とかなってたら違和感あると思う。

アーキテクチャ

DSLっぽい思想

個人的には、DSLの思想を強く取り入れたいと考えている。
DSLとはDomain Specific Languageの略で、プロジェクト内での人工言語だと思ってくれればいい。

このDSLが自然言語に近ければ近いほど、理解難易度が下がると考えている。

user.eat(rice);

DSLって言わないのは、別にドメインに限った話ではないと思ってるとかもある。
まぁこの技術ならこういう振る舞いだよねとか、ある程度自然言語っぽい記述になるといいかなと思っている。

まぁなんで、コールをみて自然言語変換できなさそうなコードは汚いと判断している。

IO分離

業務ロジックの中に！

＿人人人人人人人人人＿
＞　突然のIOコード　＜
￣Y^Y^Y^Y^Y^Y^Y^Y^Y^￣

基本的には…以下のようになるはず。

Read →　ロジック　→　Write

現実問題、途中でキャッシュに書きたいとかあるとは思うけど、ロジック中に書きたいパターンってそんなにあるか？
と思っている。

ちなみに、GUI系のアプリはなかなか上記のようにいかないのはわかる。
RESTサーバーとかだったら、できる限りきれいに書きたい。

共有カーネル

DDDでいうところのシェアードカーネルってやつ。
こいつ増えてきたら、多分、そのアーキテクチャは境界づけられたバウンダリーを引けてない。

というかシェアードカーネルが爆増する場合は、分け方を疑え。

とくにマイクロサービスとかで増えてきた場合は、マイクロサービスやめてモノシック検討したほうがいい。
ドラゴンに焼かれる。

デザインパターン

積極的に採用したい。
だいたい、先人が考えたパターンはきれい。

わからんって言われても、デザパ勉強会とかやればいいんですよ。

ただ、ユースケースが合わないところの適応には注意したほういい。

コード

いっぱい注意点ある。
思いつく限りかくけど、不十分だったりする。

import（include）（require）

まぁ、いろんな言語あるから、なんでもいいんだけど、ファイル分割した時の読み込み構文
循環参照してたら、汚い

変数への再代入

let a = 1;
for (...) {
    a = *b;
}
if(b > 100){
    a = 3;
}

a はかなり危ない。
単一責務ってクラスだけの話じゃなくて、変数も単一責務であるほうが見やすいのである。

関数が単純に長い

自分の目安として100～150に設定している。
ハイパワーな言語だったらそれより短くはなる。

自分がやってきた中で一番低レイヤーな言語のCでも150ぐらいでおさえたいと思ってる。

俯瞰してみての綺麗さ

俯瞰ってなにって話なんだけど、関数とかそのファイルを縮小表示してみて、山が高かったら汚くみえる。
マイクラとかで山はえてると削りたくなるでしょ？

平野はきれいなんだと個人的に思ってる。
せいぜい盆地ぐらいにしてほしい。

ifステートメントが３段階以上に重なっているコードはそれですぐ発見できるはず。

個人的には、四角形に近いコードが美しくみえる

理屈としてはこう…
- 大体の変数名、関数名が同じ粒度の名前になっている。
- 改行にルールがあり統一されている。
- 同じ構造が連続していて読みやすい。

ただまぁ、あくまでもこれは目安
逆にだよ、四角形になってないコードを凝視しろって話です。

再起関数

せめて末尾再帰にしたい。
というか原則禁止にしたい。

なんでか？

スタックの数、意識してコード書いてます？
スタックの数、意識してるならコメントに書いててほしい。

過去にみた、コメントについて思ったこと集

// この関数を動かすとデータが破損するかもしれません
※ 実際にはもっと腹がたつことかかれていたが、守秘義務の関係で割愛

コールグラフとかコールをみる。
呼ばれまくっとる…

そして、だいたい作者はいないので、そっ閉じ…

try {
   ...
} catch(e) {
   // nice catch!!!
}

あっ…
センスはいいけど、もみ消しやがった…

そっと、console.logを埋め込む。

// auther XXXXXXX

お前もう会社にいないんだよなぁ！
これ多い。

みかけたら、大体消してもらうようにしてる。
誰がつくったかなんて、今の時代、Gitみればわかるのよ。

// Todo: なにがし

だいたいやらないから、チケットおこしなよ。

// 何故動いているかわからない

けせｗｗｗｗｗｗって言いたくなる。

// 新バージョンに移行

新のつぎでたら、何になるんですかね？
新新ですか？

この手の、基準値比較系言語は使わないのが得策だと思ってる。
そして、こういうこと書かれてたら大体、関数名もnewとかついてたりして発狂する。

コメント書いてる時点で、読みづらいって自分でも思ってるでしょ？
なのでできるだけ、コメントを書かなくてもいい実装をこころがけたほうがいい。

一貫性一意性

同じものは常に同じ名前にしてほしい。

別名にされるとこんがらがるんですよ。
１００歩譲って、別ファイルなら許す。

同一ファイルで別名は許したくはない。

上でもかいたけど、New/Oldは注意が必要
一意性が欠如することが多い

糖衣構文

テックリードまかせでいいと思うよ。
個人的に判断基準は短い＝大正義

それは、かかなくてよい！

try {
    ...
} catch(e) {
    throw e
}

え、リカバしないの？

if {
    return xxx;
} else {
    return yyy;
}

elseいらないです。

拾われない論理構文

if(条件A){
    return xxxx
} else if (条件B) {
    retrun yyyy
}
// 何も返ってない。

LL特有現象なんかな。
型付言語だとコンパイルエラーするはず。

条件AでもBでもない場合、どうなるんやって話ですよ。

はいここでもロジカルシンキング！MECEで頼みますね！！！

コードが真っ黄色！赤もあるよ！

IDEで開いたら、真っ黄色になってるパターン
Warningオンパレードってやつ。

できる限り潰して…

最近のIDE優秀だからさ、直し方とかも教えてくれるよ。
パッと見て20個あったら、リジェクトする

ちなみに、赤は1個でアウトにしたいが、IDEの未対応だったりするケースもあるのでそこは見極めたい

deprecated

原則、MR（PR）で混じってたら、指摘対象。
過去からのバージョンアップで、含まれてしまった場合は、そっとチケットを起こそう。

自作関数を、deprecatedにする場合は、アノテーションをつけて、実行かコンパイルでわかるようにしてほしい。
決して、コメントでdeprecatedとか、かくな！！！仕組みでカバーしろ！！

似たような関数あるなぁ…

エンジニアのCtrl+Cは破壊しよう！（暴言
大体３アウトチェンジ！方式でレビューする。

くっそ長い高階関数（無名関数）

array.map(x => {
...
...
...
...
...
...
...
(下手すると５０行ぐらいあったりする。)
...
...
...
...
...
...
...
})

いや、もう名前つけろよ…
50行ぐらいあったらもう名前つけれるはず…
array.map(convert_to_report)
とかさ。

大量のローカル変数宣言

関数内にローカル変数の宣言が10個以上あったら怪しむ

よくみてきたパターン
- 書いた本人の頭こんがらがってる
- 単一責務の原則違反
- 不要なローカル変数の宣言
- struct dataclassなどデータ構造定義不足

だいたいどれかにひっかかるパターン

アクセス修飾子

private / protected / public
これらはちゃんと使い分けてほしい。

褒められた方法ではないけど、脳死でprivateが一番まし

本来こうつくってほしい。

publicにすべき関数だけ定義する。
publicの関数で、継承によって手順の戦略が組めるものをprotectedとして定義する。
それ以外の肥大化をさけるための関数パーツはprivateにする。

これが、僕が考える、privateとprotectedとpublicの使い分け。

internalとかprivate[package]はちょっと戦略がトリッキーになるから、原則上ので必要充分はみたせるはず。

final(sealed)修飾子

最終の動作するクラスには迷ったら付けとけ

const / let / var

JavaScriptに特化してるような表題になっているが、ちょっと違う。
利用の優先度とかの話。

基本的に const > let > var の順で考えてほしい。

悪とは言わない。
ただ読みやすさを考えると、変数が多いと複雑になりがち

定数だけで、プログラミングできるのが理想（絶対ないけど…）

なので、変数みたら凝視…

独自演算子

まぁどこの言語とはいわないけど、記号つかって独自演算子つくれる言語ある。
んでまぁ、極まってくるとそっちのほうが記載短いんだけど…

ちょっとこれわからんっていう状態になりがち

hoge ~~ fuga
hoge |+| fuga

まぁわからんよね…
読みやすさの観点だと、演算子つくるよりかは、ちゃんとした名前あったほうがいいと思う。

x.plus(y)

はやりすぎだけど…

メンバー習熟度とテックリードのエゴで決めればいいんじゃないかな！！

デッドコード

判明した時点で、絶対に殺してください。
それかチケット切って対応したい。

不慮の事故で生き返ってゾンビとなってあなたを襲います。

あと、美的センスを大幅に損ないます。

Tuple

public、protectedな関数ではあんまり戻り値としては使ってほしくないなぁ…
意味ある集合の場合は、ちゃんと名前をつけてほしい。

多くのパターンは、return result, errが多いかな。

return resultA, resultB, resultC このケースはツッコミはする。

使いやすいけど、無名なことに着目してほしい。

ポインタ

オワコンにしていきたい。
代替記述があるなら、そっち推奨にしたい。

std::functionとかで代用できるケースもあるはず。

無名クラス

Java系の言語にあるやつ、Interfaceを直接newできる仕組み。

var runner = new IRunner {
    run() {
        ...(実装)
    }
}

ちっちゃければいいんじゃない？ぐらいの感覚。
50行以上はだめだ！！！！

命名

プログラミングって名前付けの科学と思っておる。
だから、名前はとても重要

ぶっちゃけ、クソアルゴリズムであったとしても、命名さえしっかりしてれば取り返せる。
単体自動テストがあればなおよし。

あとは、テックリードとバチバチ議論しよう。

英語で文章になるように考えるのがコツかもしれない。

ひし形継承

下回りつくってるメンバー以外は原則させない。
ひし形継承を頭で制御できるのは作ってる本人だけケースが多い。

あとから、なんげ動いてるんだっけ？ってなること多い。

拡張メソッド

個人的にはこの表現は好き
ただ、好きときれいは別問題であって、きれいになるとは限らない。

経験上、LLではやらないほうがいい傾向がつよい
いつの間にか関数くぐったら、新しい関数ふえてたんすよねーとかありがち。
好きに追加できるからおいづらい。

個人的に、自分たちでつくったクラスは拡張メソッドしないように…
変更できないものに対して、追加しよう。

一番有効なのは、プリミティブ型の拡張だとは思っている。

if/for/while

GO言語以外の新しい言語は代替ができる。
反感かうけど、個人的にはif/for/whileぶっ殺すマン

ネスト増えるし、論理パターン増えるし…

いや、プログラミングって判断でしょ？確かにそうだが、判断少ないほうが、読みやすいんよね。

MixIn

このあたり、まぁまぁ設計難しい。
一般的なものとしては、InputStream OutputStreamをMixInしてInputOutputStreamとしたりとが有名

だからっていって何でもかんでも分解してたら、不完全クラスになる。

Reader + Writer = Repository

とかぐらいから始めればいんじゃないかな。

っと、レビューでの観点でいうと、このあたりの構造はしっかりみたほうがいい。

自分が考えるレビュアのコツ

レビュー時間かかると思ってるだろうけど
5年ぐらいやれば、早くなってくる。

最初は、めっちゃ時間かかった。

凝視ポイントを覚える

参考までに自分がつかってる凝視ポイント

俯瞰して、長い関数
俯瞰して、山脈がみえる関数
制御構文が多い
制御構文の論理構造が複雑
IOコード
.mapの中の高階関数
コメント
関数名/クラス名/ファイル名
ファイル構造

ぶっちゃけこれぐらいしかみてない。
中身のアルゴリズム？知らんよ。

プロだろ？流石にうごいてるものつくるでしょ…（ただし、3年目以降）

指摘だけが、レビューじゃない

明らかに、改善しないといけないものは指摘すればいい。
指摘以外にも、質問もしよう！

コードから仕様がよめない
意図がわからない

このあたりが、いい質問ポイントになる。

すばらしい書き方は、称賛しよう！

うおーこのクラス設計ぱねぇとか、こんな新しいかきかたあったんかとか。

実装者の迷い筆の見抜き方

ここ言語化するのすごい難しかった。
だいたい臭いで、感じてただけなので、明確な言葉にするのは初めてかもしれない

別の粒度の関数が現れる

おそらくは、当初設計してた構造から外れてしまったのだろう。
違和感のある、メソッド、関数が現れる。

いつもと癖が違う

これはもう、なんだろ…
普段からその人のコード見慣れてないと無理なんだが、いつもと違うコードだなって思うことがある。

やりたいが見当たらない

機能実現の話ではない。
コード書いてる中で、おそらく汚くしようとか思って書く人っていないとおもうのだけれども、
キレイに書こうのやりたいことが見当たらないケースがある。

それは、だいたいサボったリング

【参考】コードレビュー10年目がレビュアーとして、みてる部分をまとめてみた。