コーディング・エンジニアリングで気をつけていること

宣言的なコードを意識する

そもそもの話はこのへんの記事がわかりやすい

https://zenn.dev/mo_ri_regen/articles/declared-vs-imperative
https://zenn.dev/miyamonz/articles/3318bc87cf14cb

宣言的なコードは読みやすい。命令的なコードは読みにくい。
命令的なコードを読むときは認知の負荷が高いし、意図通りの挙動になっているか読み込む必要がある（壊れやすい）。

例えば、こういうコードは命令的なコードの例。

const source = [1, 2, 3];
const mapped: string[] = [];
for (const num of source) {
    if (num === 1) {
        mapped.push('one');
    } else if (num === 2) {
        mapped.push('two');
    } else if (num === 3) {
        mapped.push('three');
    }
}

問題点がいくつかあり、

• すべての行を読むまで、mapped にどんな結果が入るかわからない
  • 「最初は空の配列を用意して、1のときに"one"が追加されて、その次に…」と、手順を脳内でシミュレーションする必要があり、コードの読み手の負担が大きい。
  • 変換元と本当に要素数が一致するのか？　そもそも一致させようとしているのか、1,2,3だけ変換して他はなにもしないという意図があるのか？　全然わからない
  • for of はなんでも屋さんなので、ループしている以上の情報が読み取れない。
• （これは宣言的かどうかじゃなくてミュータブルなことによるけど）この変数を使わなくなったときに気づきづらい
  • mapped.push はあくまで mapped の準備であって、値の使用ではない。しかし、 mapped.push しているだけで参照されていることになってしまうので、このあとの行で使わなくなったときにTSで検出してくれない
• 先に配列の型をつける必要がある
  • 空配列だけ宣言すると never[] に推論されるので、 string[] などと付ける必要がある
  • つまり型推論に任せることができない。変数の型は極力書かなくて済むのが理想である。
  • 広い型をつけてしまいがち。本当なら ("one" | "two" | "three")[] でいいはずなのに、 string[] にしてしまう。

これを宣言的なコードにしてみよう。
GitHub Copilotに「宣言的に書いて」とお願いするだけで意外と書き直してくれたりする。不慣れな人は試してみてほしい。

const source = [1, 2, 3];
const mapped = source.map(num => {
  switch (num) {
    case 1:
      return "one";
    case 2:
      return "two";
    case 3:
      return "three";
    default:
      return "";
  }
});

これのいいところは命令的なコードの問題点の反対で、

• map を使うことで、「配列の各要素を1対1対応する形で変換（マッピング）する」ということが伝わる
  • たった3文字にこんな意味が読み取れる素晴らしさ 🎉
• デフォルト値（1,2,3以外に対する対応が明確になる）
  • この例では defalut: return ""; としているが、もしこれを入れなければ undefined が入ることになる。それが型システム上検出できる。
• イミュータブルな記述になることで、 mapped を使わなくなったらすぐ気づける
• map のコールバック関数から、型推論してくれる
  • 型注釈を1個も入れなくてよい

宣言的に書く例をいくつか。

const source = [1, 2, 3];
let count = 0;
for (const num of source) {
  if (num % 2 === 0) {
    count = count + 1;
  }
}

宣言的に書くとこう。これもGitHub Copilotに「宣言的に書いて」とお願いするだけで変換してくれた。

const source = [1, 2, 3];
const count = source.filter(num => num % 2 === 0).length;

filter には「配列内の特定の条件に当てはまる要素に絞り込む」、 lengthには「要素数を数える」という目的があるので、この2箇所を読むだけで何をしたいかが把握できる。
この例では、 let だったのを constに置き換えることができた。 let だといつどこで書き換えられるかわからないため、読み手の負荷を上げるキーワードである。できるだけ使わず、 const にできるとよい。

配列の話だけではなく、他のものも同じ。

let operation: string;
if (exists) {
  operation = "update";
} else {
  operation = "create";
}

const operation = exists ? "update" : "create";

イミュータブルに書く

これも宣言的に書く話と近いのだが。イミュータブルな処理で書かれていると読みやすい。
イミュータブル自体の話は世に記事が色々あると思うのでそちらへ。

https://sbfl.net/blog/2018/09/25/javascript-immutable-programming/

TypeScriptでは、 as const や readonly , Readonly<T> などを使って実現する。
イミュータブルであれば、同じ変数名がどの行で出現したとしても同じ中身だと思って読める。これがミュータブル＝変数の値が書き換わる処理になっていると、「この行でこれが書き換わったからこの行ではこういう値で…」と、脳内でシミュレーションしながら読む羽目になってしまう。

型はつければいいんじゃなくて

TypeScriptにおける話。
他の人が書いたコードを見ていると、TypeScriptのコンパイルが通ることで満足しているケースが多い。十分に（必要最小限にまで）型は狭くするべきである。

実例は、以前投稿した「TypeScript問題集」の第3章の問題群である。
これらは「TypeScriptのコンパイルは通る状態になっているが、よりよい型付けができる例」を取り扱ったもの。
https://zenn.dev/kagan/articles/typescript-practice#第-3-章-より安全な型へ

型はドキュメントである

自分の中で大事にしている意識が「型はドキュメントである」ということ。もう少し言い換えると、「型がドキュメントになるように型を付けろ」ということ。
型を見ればそこにどんな値が入るのか、どんな使い方をするのかわかる状態にしておく。

例えばこんな例。型で string を指定しながらコメントによって補足している。コメントによってドキュメンテーションを加えている例である。

/** URL文字列を入れてください */
const url: string = "https://example.com";

そんなことをするより、型で指定してしまおう。
これならば別でドキュメンテーションすることは不要だろう。これだけで人間にも伝わるし、なんたって機械的にチェックすることができるから。

const url: `https://${string}` = "https://example.com";

命名はめちゃとても大事

名付けは人間の認識にも影響する。
名前のついてないものは認識することが困難なので（ジョシュアツリーの法則）、そもそも名前のついていない事象・概念に対して命名することが必要なケースがある。
命名をする際は、制作者の意図が読み手に伝わるように意識する。

命名の観点については別のスクラップに書き出している。
https://zenn.dev/kagan/scraps/c5df40eeeda99f

コーディングにおけるデザインセンス

デザイン4原則の「近接」「整列」「反復」「対比」はコーディングにも活かせる。

近接

関連するコードは近くに書こう。
逆に、関連しないことを示したいときは離そう（空行を入れるなど）。

const characters = ["a", "b", ""];
const filteredCharacters = characters.filter((item) => item);
const numbers = [1, 2, 3];
const filteredCharactersCount = filteredCharacters.length;
const filteredNumbers = numbers.filter((item) => item);

const characters = ["a", "b", ""];
const filteredCharacters = characters.filter((item) => item);
const filteredCharactersCount = filteredCharacters.length;

const numbers = [1, 2, 3];
const filteredNumbers = numbers.filter((item) => item);

整列

同じネストレベルの行は行頭を揃える。
フォーマッターを使っている限り自動でインデント揃えられるからわざわざ意識しないけど。

反復

同じようなコードは同じように書こう。

function funcA() {}
const funcB = () => {};

const funcA = () => {};
const funcB = () => {};

対比

ファイルがフラットに並んでいると関係性が分かりづらい。

├── bar.ts
├── foo.ts
└── main.ts

メインとサブを分けると関係性が読み取りやすい。

├── main.ts
└── modules/
    ├── bar.ts
    └── foo.ts

可読性の高いコードの書き方に関して、やり方がよくわからないという場合は『リーダブルコード』を読むといいだろう。

https://www.amazon.co.jp/dp/4873115655

テストは失敗することのほうが大事

開発の中で、追加したテストが成功するパターンしか確認していないときは注意。
実はそもそも実行されてなかった、実装に関係なく成功するテストになっちゃってた、とかが考えられる。
Red→Green→Refactorのサイクル（実装を書く前にテストを書いて、テストが失敗することを確認する流れ）が望ましい。
先に実装を書いてしまった場合、一度実装コードの一部（ここの処理がなくなったら壊れるという箇所）をコメントアウトしてみて、テストが失敗することを確認するとよい。

テストをなんで書くかと言ったら、（将来的に）実装が壊れたときに検出するためのもの。今テストが通ることだけを目指したら意味がない。

テストは願いを書くもの

テストには、「こういう引数を与えて実行したらこういう値を返してほしい」「こういう引数を与えて実行したらエラーを返してほしい」など、実装者の意図・願いを書く。
何を確認しないといけないかを意識するのも大事。

テストはドキュメントになる

テストを見たら、そのテスト対象（関数など）の使い方がわかるようにする。
人間に対する可読性もある程度意識するとよい。

ライブラリのドキュメントをイメージしてもらうとわかりやすい。
たとえばunderscore.jsのドキュメントを見てみると、関数ごとに入力の例とそれに対する出力が書かれている。このイメージ。

_.first([5, 4, 3, 2, 1]);
=> 5

_.last([5, 4, 3, 2, 1]);
=> 1

テストを十分に書いておくと、わざわざコメントやドキュメントで「こういう値を入れてください」とか「こういう値が返ってきます」とか書く必要がない。そういったドキュメントは腐ることが多く、実装が変わったときに更新されない可能性が高い。
一方テストは機械的に実行され、実装の変更に追従していない場合エラーとして検出できる。

自作自演テストに気をつけろ

「自作自演テスト」とは、t-wadaさんの講演で出てきた言葉。
テストコードをプロダクトコードと同じロジックで計算しているようなケースを言う。
これだとテストの意味がない。

https://levtech.jp/media/article/column/detail_496/#:~:text=ラスボスです。-,3つ目は「自作自演」。,-テストとコード

レビューをする際には、テストコードもしっかりレビューする必要がある。

レビュイー（プルリクを出す側）として

• 可能な限りプルリクを分割する
  • 差分が少ないほうが目的が明確になり確認しやすい
  • なにか問題が起きたときの切り分けもしやすい
• 出す前にセルフレビューする
  • レビューしてもらう前に気付ける問題は消しておく
• コードを読んで疑問に思いそうなところは、補足する
  • そもそも、今後そのコードを読んだ人が同じ疑問を抱きそうなら、コード中のコメントとして書いておく。
• プルリクの説明文は、将来見返したときに意図が伝わるようにする
  • 今はチームメンバーが全員状況を把握しているから意図がわかるプルリクでも、将来見返したら「なんでこんな変更したんだっけ…？」となるのを避けたい。
• 実行して確認してほしい内容を明示する
  • 「正しく動作することを確認する」は説明になってない。正しく動作してほしいのは当たり前。何をもって正しいと判断できるのか説明する。
  • UIを含む場合、スクリーンショットを添付するとよい。レビュアーが「表示されたからOK！」と思っても、実は実装者の意図する表示と異なる表示になっているかもしれない。
  • 確認に必要なローカル環境のURLを貼っておく

レビュアーとして

• レビューコメントにはラベルを付けて、優先度が伝わるようにする

以下の記事を参考にしつつ、カスタマイズしている。

https://zenn.dev/hacobell_dev/articles/code-review-comment-prefix

英語は覚えるまでの負荷が高いので、日本語にしている。

【必須❗】
【提案】
【質問】
【参考まで】
【細かいことですが】

これ以外にも、

〇〇が✕✕なのですが、～～～なので問題ないです。

みたいなコメントは、「結局問題ないんか～～い」とツッコミを入れたくなってしまうコメントだと思うので、以下のように前置きしたりします。

（対応不要ですが、一応コメント）
〇〇が✕✕なのですが、～～～なので問題ないです。

• 修正をお願いしたいときは、修正指示をセットにする
  • 自分も気を抜くとやってしまうが、修正依頼（GitHub の Request changes）を出しているにも関わらず、具体的な修正指示がないコメントをたまに見る。「〇〇は使わないでください」とか「追加された〇〇というファイルが使われていません」とか。それぞれ、「なので別の✕✕を使ってください」、「なのでファイルを削除してください」という指示が暗に含まれているはずです。だったらそれを書きましょう。自分（レビュアー）が意図したものとは違う変更がなされて返ってくるかもしれません。レビュイーが修正内容を考えられるくらいスキルのあるメンバーならまだいいですが、ジュニアメンバーの場合はさらに負担が高まります。「このくらいわかるよね？」という意図を感じ取ってしまい、わからなかったとしても質問しづらくなってしまうかもしれません。
• 修正をお願いしたいけど案が出てこない場合は、それを伝える
  • 「〇〇だと紛らわしいため、違う名前がいいかもしれません。とはいえ私もいい案がないのですが…」とか
• コード差分以外の部分を意識する
  • どうしてもコード差分を読んで満足してしまいがちだが、それだと漏れていることがある
    • この変更により使わなくなった変数を削除していない（※これは knip を使って機械的に検出したほうがいいけど）
    • 同じ変更すべき箇所があるが、対応されていない

そもそもを疑う

要望をそのまま実現するのではなく、本当にやりたいことをイメージする。
「ドリルがほしいんじゃなくて穴がほしい」的なこと。

「〇〇機能を実装してほしい」という依頼があったとして、その依頼者の目的を達成するのは〇〇機能だけとは限らない。
目的を理解したうえで、より効率的に、より少ない工数で実現できる方法がないか考える。

一度立ち止まり、俯瞰して考えてみる

実装を進めていると、その方法しかないものと信じて疑わないようになり、視野が狭くなってしまいがち。よく考えたら別の手段があるかもしれないのに。
視野が狭くなりがちなことを自覚し、たまに一度立ち止まり、俯瞰して考えることを意識する。

チーム内の問題としては、そういう「これってそもそも…」な指摘が実装後に来てしまうと手戻りが発生してしまう。そのため、実装方針はできるだけ早めに共有しておくとよい。