🔥

はじめてレビューする人に毎回似たようなことを説明するので

2022/01/18に公開約7,000字

レビューするときにどんな観点でレビューしたらよいかわからないことが多いと思います。そんなときにだいたいこのことを理解しておいたら大丈夫と言う観点をいくつかピックアップして紹介します。

こちらにまとめておきます。

  • わからなければきく!
  • リスクを理解する
  • 異常系のハンドリングを学習する
  • わかりやすい命名をする
  • 変数のスコープと宣言する場所
  • 参照を渡しているのかどうか
  • 依存の形と数の関係を理解する

わからなければ聞く!

これは一番重要です。レビューはじめての頃は、全てを理解して動作保証までレビュワーの責任だとおもってしまっているかもしれません。ただそれは間違いで、レビューはなんとなく良いかまで見て最終的にはLGTM(Looks Good To Me)私が見る限りはいいと思うよ、というところまでがレビューするときの役割です。

「私が見る限りはいいよ」というも状態に持っていくときに、自分が知っている知識や背景でレビューができるならよいのですが、そうではない場合があります。

例えば、フロントエンドに詳しくないけれどレビューをお願いされたときなどです。フロントエンドのマスターになってからレビューをするのでしょうか?そんな時間ありませんよね?なので、そういう場合は実装者にどういう懸念や選択肢があって、どうしてそれを選んだかを聞いた上でレビューします。そうすることで、背景や文脈を理解してレビューをすることができます。

ここで一点注意なのですが、レビューする上で最低限のことはしっかり見るということは忘れないでください。最低限のことは以降の段落で紹介します。

リスクを理解する

ここで言うリスクとは具体的にはシステムがビジネスに与えるリスクです。

具体的に例を上げていきます。

例1. 商品購入ページでエラーが起きた場合

商品購入されることでサイトの運営者に利益が発生している場合に、商品購入ページでエラーが起きるのは売上に直接影響します。加えて、もしECのモール型のサイトでそこに別の会社が商品を出品している場合は、出品者の利益にも関わるので信用を失いかねません。

例2. 自社サイトのFAQページでエラーが起きた場合

FAQページは、あったら嬉しいものですしSEOなどには少しは貢献するかもしれません。ただ、そこがエラーで見れなくなったからと言って売上には影響しません。

例3. ローカル開発環境用のdocker-composeを修正する

ローカル開発環境用のdocker-composeを修正しても開発者が困るだけですね。

例4. 本番環境用のterraformを更新する。

本番環境のインフラ構成をterraformで更新するとします。この場合は、もしエラーがあってアプリケーションが全然動かない、外部に対して秘匿情報が公開されてしまうということがあれば会社の存続の危機にもなりうる大惨事です。可能性としてはありえません。ただ、翌々考えてみたら、外部に秘匿情報が公開される可能性のない箇所の変更、CI、CDの変更であれば稼働しているアプリケーションには影響はありません。加えて、アプリケーションと違って、プロダクション環境とテスト環境でデータの容量がちがったり、イレギュラーデータが存在してエラーになる可能性も低いと思われます。なので、修正内容によってはビジネス的には全くリスクが無いこともあります。

といったように、エラーが起きたときに何が生じるかでリスクはある程度把握できます。例3. であげた内容のレビューであれば少し見ただけでもしエラーが有ったとしても新しく修正差分を作ってもらったいいというくらいでレビューもできます。(私はそうしてます。)

大切なのは、リスクの大小を考えてレビューをどの程度しっかりするのかを考えることです。すべてのレビューに全身全霊を掛ける必要はありません。

異常系のハンドリングを学習する

異常系のハンドリングは、経験の浅いうちにはよく迷うところです。ただいくつかのポイントを抑えておいたらそこまで難しいものでもありません。

最初に考えるべきかポイントは、エラーが起きたときに握りつぶすのか、エラーを表示させるのかです。

エラーが起きたときに握りつぶす場合

握りつぶす場合というのは、いくつかの文脈が存在します。まずは、技術的な仕様とビジネス的な要件の折り合いを付ける場合があります。たとえば、なにかのデータを1件だけ取得するときAPIがあったとしましょう。

一件データを取得するAPIで一件もデータがない場合は、だいたいの場合はHTTPのステータスでエラーとなるように実装します。

ただ、そのAPIをどこかのページからサブコンテンツとして呼び出して使用するときにはエラーにしたくないことの方がほとんどです。そういった場合にエラーはtry~catch 補足してしま宇野が良いでしょう。

あとは、そのエラーがおきてもアプリケーションがエラーを起こしていると知らせたくない場合です。

例を上げるとキャッシュ系の処理です。アプリケーションを高速に表示させるためにキャッシュの仕組みを導入したとします。ここで高速化とアプリケーションが正常につかえることのどちらのほうが重要化考えてみましょう。アプリケーションが正常に動かないとそこからコンバージョンなどもできませんし、売上にも繋がりません。一方で高速化をした場合には、0.1sec速く表示させるたびにその導線からの売上が1.1倍に上がるという報告もあります。売上が0になる可能性と1.1倍になる可能性であれば、0になる可能性をなくすのがまともな思考です。

結論、キャッシュ系のサーバーが一時的にエラーを起こしたとしてもそのエラーは握りつぶしてキャッシュを使わないようにページを表示させるべきです。

あとは、例えばですがABテストのような機能を実装したときも同じです。ABテストはあくまで新しい機能やデザインをどのように実装しますが、その機能がエラーを起こしたとしてもユーザーが通常機能は動くするように作るべきです。そういった場合にエラーを握りつぶすという判断をします。(Fail Softに実装するとも言います)

後はやむを得ない事情で、エラーが起こっていてもその通知を受け取っていたらエンジニアの方で復旧可能な場合などは見かけ上処理が成功しているように見せるためにすることもあります。なのでtry~catchでくくって、catchブロックで通知だけを実装します。

エラーを表示させる場合

よく目にするのは、対象のリソースが無いというエラー表示と、入力値が間違っている場合のエラー表示、開発者が予期していない事象が原因で起こっているエラー表示です。

対象のリソースが無いというエラーと入力値が間違っているというエラーは、しっかりテストさえしてあればユーザーが入力した値が誤っていたというものなので、意図的に引き起こさなければ起きないものと認識してしまって良いでしょう。ただ、サイトが大きくなるにつれて、入力値の実装を変えてしまって、調査に見落として誤った入力値で入力されているということもあるので、入力値が間違っている場合のステータスコード400系がどれくらい生じているのかを監視する仕組みはあったほうがいいでしょう。

開発者が予期していない事象でエラーが起こる場合は、だいだいの場合にページ上にも「エラーが起こりました。」「しばらくしてからアクセスしてください」のような文言が表示されます。こちらはその原因が何故起こったのか確認して必ず対処する必要があります。スタータスコード500のエラーになります。

ここでtipsとして意図的に500系のエラーを生じさせる場合もあります。前提となる条件は、テストコードがなくて、何かしらのライブらいのインターフェースの構造上実装メソッドを実装しておかなれけばいけない場合です。

テストされて無い実装をリリースさせるわけには行きません。だけど、そのメソッドを呼び出す処理はありません。

そういった場合に独自のExceptionを作成したりします。

class MethodNotImplementedException extend Exception {
  constructor() {
    this.super()
  }
}

interface HttpClientInterface {
  get: Function
  post: Function
  delete: Function
}

class HttpClient implements HttpClientInterface {
  constuctor() {
  }
  // deleteに関してはこのリリースで参照されることはないけど、読み込んでいるInterfaceがdeleteも実装することを強制してしまう。
  delete() {
    throw new MethodNotImplementedException();
  }
}

そう言って感じに、httpClientのテストで、application/jsonのContent-Typeの挙動は見るけど、それ以外のリクエストが来た場合はエラーにしておいて、その機能を使う別の開発者がテストされてない機能を参照しようとしたときに明示的にエラーにして開発の途中で気づかせるというものです。

わかりやすい命名をする

わかりやすい命名と言っているのはここでは、冗長な命名のことをさします。エンジニア経験の浅い人でも長い人でも、変数や関数の命名が短いのがいいと思っているヒトを結構見かけます。短い命名で変数と関数やメソッドの処理の内容がわかったらいいのです。がしかし、命名に時間をかけて渾身の一撃のような文字列をひねり出すよりなら長い説明的な命名をしてしまったらいいと思います。

例えば次のような関数について考えてみましょう。会社情報のリストから、会社の所在地を取得するような関数を作ります。

getAddresses(){
}

getAddressesFromMakerList(){
}
``
どちらの関数のほうがわかりやすいでしょうか、上の関数と下の関数では情報量が違いますよね?もしかしたらgetAddressの関数を作ったヒトは、会社リスト以外でも使われることを想定してそのように作ったのかもしれません。ただ、そうだとしても実際にその背景を理解してそのように追加実装するかどうかはわかりません。十中八九その意図が理解されることは無いのではないかと思います。

その一方で下の関数は関数の名前通りに使う以外の用途が思い浮かびません。ただ、先程のように今必要なのがそれなのであればそれに特化して作ってしまって、後で必要があったら他のデータリストにも適応できるようにしておいたら良いでしょう。

同じようにbyをつかってどのようにデータを取得するのかものイメージを沸かせることもできます。

getMakersByXXX(){
}

このときにあまりにも非効率だなとなったら、getMakers(){}としてどの検索条件で取得できるかを呼び出す側からパラメータとして指定できるように作ってあげたら十分だと思います。

余計なことは考えずに用途を限定して、どんどん改善させていきましょう。

## 変数のスコープと宣言する場所

変数のスコープとブロックの関係を見ましょう。ブロックスコープを持つ言語であれば関数の中for文の中、if文のブロックの中で宣言したものはそのブロックの内部でしか使用できません。その性質を利用して可読性の高いコードを書けているかが重要になります。

極端な例を示しますが、1000行を超える処理が一つの関数またはメソッドに書かれていたとします。その関数の中ではいろんな変数が使われていることでしょう。その中で変数のスコープが適当な場合と使う範囲に限定されている場合はどちらがよみやすいでしょう。

// 1. 1000行を超える変数のスコープが誤ってしまっているパターン
main() {
let 変数A;
let 変数B;
let 変数C:

if () {
// 変数Aはここでしか使用されない。
}

for {
// 変数BとCはここでしか処理されない。
}
// 変数Cを使った処理
}

// 2 .1000行を超える関数変数のスコープが正しいパターン
main() {

if () {
// 変数Aの処理
const 変数A;
}

let 変数C; 
for {
// 変数Bと変数Cを使った処理
const 変数B;
}
// 変数Cを使った処理
}


1のパターンと2のパターンでは、明らかに2のパターンのほうがプログラムを読むときに読みやすいです。どこが読みやすポイントなのかというとそのブロック内でしか変数を使わないときにしっかりその変数をブロック内で宣言することでそのブロックを出た後はその変数のことを忘れることができます。あとは、変数Cはmain関数の中で繰り返し使われる変数であるので、使用する直前に宣言することで、読み手にとってはこれから変数Cが使われるというのとその後の処理でも参照されるという情報を得ることができます。

## 参照を渡しているのかどうか

mutable(可変)とimmutable(不変)についての話になります。細かい説明は今回は省きますが、mutableな変数とimmutableな変数ではimmutableな変数を作って処理するほうが全体的にアプリケーションのメンテナンス性が高いという前提で話します。

DDD(ドメイン駆動開発)を実践している場合は、アプリケーションでValueObjectをつかってなんたらかんたらの話をすることも必要なのですがほとんどの場合はそれには当てはまりません。

殆どのアプリケーションではDDD(ドメイン駆動開発)は実践されてないでしょうから参照を渡しているかどうかについて確認できていたら良いでしょう。

function createNewValue(val){
val.a = "sample3";
return val;
}

// begin
const original = { a: "sample", b: "sample2"};
const changed = createNewValue(original);
// end

上のコードでbeginとendと書いたところに注目してみてください。refという変数を使って新しい変数を生成しているように見えませんか?そしてoriginalにはきっと変化がないだろうとそう思うのではないでしょうか?

ですが実際にはoriginalの変数はvalueChangedの中で書き換えられてしまっています。ということは、originalにどんな値が入っているかはcreateNewValueの中身も見ないとわからないことになります。そういったことを防ぐために関数に渡されている変数を理解してメンテナンスしやすいコードを書く必要があります。例えばこんな感じです。

function createNewValue(val){
const clone = Object.assign({}, val);
clone.a = "sample3";
return clone;
}

このようにして書くことで、originalの変数の値をbeiginとendの間で書き換えることなく実装ができます。こうした積み重ねが読みやすいコードとメンテナンス性の高いシステムの構築に寄与します。

## 依存の形と数の関係を理解する

依存の形とは、AとBの関わり方を言います。所有の関係(A has B)、利用関係(A use B)という関係ですね。あとは、数の関係です。一対一の関係、一対多の関係、多対多の関係ですね。クラスとクラスの関係においては必ず確かめてください。共通化する関数などを作る際にも上記のことを考えてください。

抽象化した概念と実際のクラスの数の関係がずれてたりするとそれはもう大変なことになります。

## まとめ

どうでしょうか?レビュー初心者の方が気をつけることを抽象化してまとめてみました。ドメイン知識などに関しては、体系化して説明することはできませんのでその職場職場によると思いますが、基本はこの辺の知識を頭に入れてレビューができていたら数を重ねていくうちに自分のなかで知識が体系化されてより洗練されたレビューが行えると思います。

Discussion

ログインするとコメントできます