新人のコードレビュー時に意識している事 〜悲しみの連鎖を断ち切りたい〜

はじめに

多分に宗教の違いもあると思いますので、しがないエンジニアの愚痴・戯言程度に流していただければと思います。

前置き

今年はいろいろと他人の作ったもの(主にweb)を新たに運用保守する機会が多かったです。
そしてどれも見事なク◯コードだったので、私のような被害者を出さないためにも今後コードをガリガリ書いていくであろう新人の育成には力を入れようと強く心に誓ったのでした(遠い目

そのため本稿では、今年の地獄を経た私が、どのような観点で面倒を見ている新人のコードレビューをしているか書きます。
滅びよ全ての◯ソコード。

主にPHP・JavaScript使ってます。

観点

主に5つあります。
コメントを書けとかそういうのは大前提なので省略します。

その1. switchを使え

条件分岐は何でもかんでもif。これはかなり多くて驚きました。
ifだと評価式にどんなものでも入るので、ひと目では規則性・意図が読めません。
実は間のelse ifで評価している変数が1文字違い、意味が違った、なども起こりえます。
switchだと何に対しての評価だというのが一意なので、余計な可能性を考えなくてすみます。

範囲などでswitch(true)を使うのは、個人的には好きですけど、さすがに宗教が過ぎるかなと思って書かせはしません。(見つけたら読めるようにはなってほしいけど)
スコープ区切ってコメント付けて、後から見た人が何の範囲を評価したいのか分かりやすく書けって感じですかね。

// こっちよりも
let input = 'apple';
if(input == 'apple'){
  console.log('りんご');
}else if(input == 'orange'){
  console.log('みかん')
}else{
  console.log('果物ではない');
}

// こっちが良い
let input = 'apple';
switch(input){
  case 'apple':
    console.log('りんご');
    break;
  case 'orange':
    console.log('みかん');
    break;
  default:
    console.log('果物ではない');
    break;
}

その2. 変数の初期化は直前でしろ

人によるかもしれませんが、私の場合はコードは基本的に出力(or レスポンス)から追っかけます。
入力(or リクエスト)がどのように使われているか、を追いかけるより、どのように出力が生成されるか、を追いかけた方が効率的と思っているからです。
極端な話Webだと使ってないリクエストパラメータがあるケースもありますし()
そうなると、意味も無く先頭で初期化された変数は、その先頭からレスポンスまでの広範囲で加工されている可能性のある厄介なものとなります。
switchにも通じるものがありますが、余計な可能性を簡単に無くせるなら無くすべきだと思います。

// こっちよりも
let work1 = 'apple';
let work2 = 'orange';
let work3 = 'peach';

if(work1 == 'apple'){
  console.log('りんごがあるよ')
}

if(work2 == 'orange'){
  console.log('みかんがあるよ')
}

if(work3 == 'peach'){
  console.log('ももがあるよ')
}

// こっちが良い
let work1 = 'apple';
if(work1 == 'apple'){
  console.log('りんごがあるよ')
}

let work2 = 'orange';
if(work2 == 'orange'){
  console.log('みかんがあるよ')
}

let work3 = 'peach';
if(work3 == 'peach'){
  console.log('ももがあるよ')
}

その3. 入力は先頭で格納しろ

上記に矛盾するような気もしますが、inputは先頭で受け取って変数に格納してほしいです。(コマンドのオプション、Webのリクエストパラメータなど)
特にインターフェイスが変わったときが本当にめんどくさいので…
バリデーションや加工も先頭でしてたら分かりやすいですしね。
というか先頭でしない・できない理由ってあまり見たことないので、逆に先頭に統一されてないと何かあるのか？というまた余計な可能性が(ry

その4. 三項演算子を使え

三項演算子は使い方がおかしいとその分かりにくさたるや凄まじいものがありますが
使い方次第ではとても便利だと思います。
ifでやたら条件を見てはこっちの値、それ以外ならこっちの値、みたいなのがズラーッと列挙されてたら読む気を失います。
ブロックまで含めて5行、三項演算子なら1行で書けます。
こういう、単純に評価式を見て値を切り替えるだけのものはガンガン活用していった方が読みやすいです。

// こっちよりも
let input = 'apple';
if(input == 'apple'){
  console.log('りんごがあるよ')
}else{
  console.log('りんごがないよ')
}

// こっちが良い
let input = 'apple';
console.log((input == 'apple')?'りんごがあるよ':'りんごがないよ');
}

その5. スコープを分けろ

あえて関数化しろではなくスコープを分けろと言います。
関数のメリットは処理の塊を明確にできる事と、処理の再活用だと思っていますが、単に前者だけの目的で使われている(と思われる)関数達をたくさん見かけます。
前者だけなら、無名関数を即時実行するだけで分かりやすいじゃんと思うのです。
※ただし数行の処理に限る
もう意味もなくメインロジックからたらい回しにするのは勘弁してくれさい

このへんをきちんと意識できるようになると、共通化とか拡張性のあるコードの構造とかそのあたりを考えられるようになるのかなと思います。

あとがき

あくまでもドキュメント・仕様書はたまにある、というレベルのプロジェクトで経験したものを元にしています。
コードもコメントが無いのは当たり前、何を書いてるか分からないコメントですらトリックの推理に使える貴重なピースとなるレベルです。
※もちろん犯人の用意した罠の可能性もある

いつかそんなプロジェクトが滅びてほしいと思いつつ、まずは少しでも周囲のコーディングのクオリティアップのために今後も私なりに種を蒔いていこうと思います。