Lycolog

単体テストを書くことには賛否両論あると思うが、私は書いたほうが良いと考えている。これは単に単体テストが書けるからだけに限らず、様々な恩恵があるからだ。

今回は単体テストを書くことで、副次的に得られる恩恵を書いていく。ここではクラスを使わず、関数で実装していくタイプの開発にフォーカスして話す。Reactの関数コンポーネントのような、関数型ではないが、クラスは使わないみたいなやつだ。

関数が小さくなる

単体テストは関数やクラス単体の振る舞いを見るのが目的だが、1000行あって、中にクロージャや分岐、ループがネストされていると非常にテストがしづらくなる。つまり単体テストをしやすくするために関数のサイズは必然的に小さくなる。理想的には関数が持つ責務が一つになるはずだ。

関数が小さくなれば変数のスコープも短くなるためコードの見通しが良くなり、長大な変数名を減らせるなど、いろいろメリットがある。極端な話だがGoでは1文字変数が市民権を得ていて、これは関数のスコープを短くする文化があるので成立している。

疎結合になる

単体テストを書く場合に密結合だとテストが書きづらい。そのため疎結合にするようになる。具体的に言うと依存性を逆転させるようになる。依存性が逆転していると、依存を注入できるためテストがしやすくなるのだ。

例えば以下のプログラムはDate ()をモックする必要が出てくるので専用のライブラリを入れたり、何かしらのハックを使う必要があるだろう。

const isOneDayLater = (time: number) => {
  return (+new Date() - time) > 86_400_000;
}

しかし、以下のように外部から値を注入すればモックしなくともテストコードを書くのが容易になる。第二引数に値を入れればいいからだ。

const isOneDayLater = (time: number, baseEpochTime: number) => {
  return (baseEpochTime - time) > 86_400_000;
}

コードの見通しが良くなる

例えば次のような関数は冗長だ。この程度の長さならまだしも、これが500行、1000行と増えていくと読むのが大変になる。現実ではもっとネストが酷かったり、前後に長大関数がいて、注意深く読まないと解読不能ということもままある。そしてこの関数をテストするのは書いてある処理すべてを検査する必要があるので、テストコードも長くなり、しんどい。以下のようなコードを私は「べた書き」と呼んでいる。

type User = {
  id: string;
  firstName: string;
  lastName: string;
  gender: string;
  address: string;
};

export const registerUser = async (user: User) => {
  if (!/^\d+$/.test(user.id)) {
    throw new Error('ユーザーIDの書式が不正です。');
  }
  if (user.firstName === '') {
    throw new Error('姓が入力されていません。');
  }
  if (user.lastName === '') {
    throw new Error('名が入力されていません。');
  }
  if (user.gender === '') {
    throw new Error('性別が入力されていません。');
  }
  if (user.address === '') {
    throw new Error('住所が入力されていません。');
  }

  const resp = await fetch('https://example.com/api/v1/user', {
    method: 'POST',
    body: JSON.stringify({
      id: Number(user.id),
      name: `${user.firstName} ${user.lastName}`,
      gender:
        user.gender === '0' ? 'male' : user.gender === '1' ? 'female' : 'other',
      address: user.address
    })
  })
    .then(async (resp) => await resp.json())
    .catch((e) => e);

  if (resp.status === 200) {
    return resp;
  } else {
    const payl = resp.json();
    return {
      errorMessage: payl.errorMessage;
    }
  }
};

しかし次のように書けば34行あった関数が9行にまで減り、見通しが良くなる。関数の中で何をしているかわからないという声もあるだろうが、関数名から内容を読み取れるようにしておけば問題にならない。読み取れない処理を入れなければいいのだ。こういう風にべた書きを一定の粒度で関数化することをモジュール化と呼ぶ。

type User = {
  id: string;
  firstName: string;
  lastName: string;
  gender: string;
  address: string;
};

export const registerUser = async (user: User) => {
  validateUser(user);

  const payload = createRegisterUserPayload(user);

  const resp = await requestRegisterUser(payload);

  return validateRegisterUserResponse(resp);
};

例えばvalidateUser()の中でuserバリデーションと関係ない処理、例えばサービスプロバイダを通して値の授受をしたり、どこかのAPIを叩いてたりすると、全く信用できなくなる。これは設計段階で関数の中身を単一責務にする制約を課すことで回避できる。開発標準に盛り込んでおくことが望ましいだろう。

単一責務で関数にまとめると、単体テストはその関数単位にできるためスコープが狭まり、テストコードが書きやすくなる。親の関数はどうするのか？という疑問が出るが、これは関数を呼んでいるかや、処理フローが正しく流れるかを検査すればいい。実際に関数を呼ぶ必要もなく、呼び出し関数はモックでよい。そうでないと結合テストになってしまうからだ。ただし問題もある、モック化した関数を呼び出すと本来と違う振る舞いをさせることもできてしまう。

これは実装やテストコードで型を騙さないことである程度防ぐことが可能だ。型が一致していない場合エラーとなるため、インターフェースレベルで不正なテストが書かれてしまう可能性を減らすことができる。勿論、どれだけやってもモックを呼び出しているのでテストが不正に通過する可能性はある。

テストが不正に通過する可能性を低めるためにはテストコードを書いたときや修正したときに、意図的に落ちる実装に変更してテストが落ちるかどうかを確かめたり、カバレッジの変化を見たり、結合テストを書いたり、手動での動作確認をするのが有効だ。

自動テストを書いているのに手動で確認するのかとか、結合テストを書いたら二重テストになって無駄ではないかというのもあると思うが、どちらも網羅的にする必要はなく、クリティカルパスが通っていれば、後の単体テストは問題なく通ると考えてよい。要するに正常系と異常系の2パターンを見て問題なければ、単体テスト側も問題ないだろうという考えだ。

では、そもそもの視点が間違っていたらとか、それでも単体テストが間違っていたら…というのも勿論ある、あるのだが、無限に追及していると際限がなくなるし、結果論として網羅的でバグのないe2eテストという非現実な解が出てきてしまう。単体テストはあくまでその製造コストの低さから条件を網羅しやすく、高速に動作するため、不具合の早期発見や、開発中の微修正を起因とした手動フルチェックを防いだり、何が書いてるのかイマイチわからない巨大な結合テストコードを減らすのに貢献するもので、バグをゼロにするためのものではない。

同じことを何度もテストしなくてよくなる

レビュー指摘などでロジックを直したり、不具合に気付いて直したりしたときに毎回全数テストをするのはどう考えても非効率である。しかし単体テストがあれば、その工数を減らすことは可能だ。特に分岐網羅に対しては非常に強力なアドバンテージがある。仮に分岐パターンが4個程度しかないとしても、手動でやっていては効率が悪い。コード直して起動するだけで手間なのに、更に操作とかやってられない。

手動テストはなくならない

手動テストが減ることはあっても、なくなることはない。何故なら初回実装時はそもそも動くかどうかすら解らないのだから、この時ばかりは見る必要がある。手動テストで問題がなかったからこそ、以後の単体テストが信頼できる状態になる。端から動いていなければ、テストコードは意味をなさない。過去に手動確認し、その時に通っていたテストコードだからこそ信頼できるのである。

ちっとも眠気が来ないのでトップページを弄くり回していたのだが、トップページが概ね完成した以来の大規模更新となる更新を行った結果、Lighthouseのスコアが100になった。

主な更新としては2カラムを1カラムに落とし、職歴系の記述をガっと消した感じ。趣味のサイトなのに仕事のことあんま書きたくないなというのと、書いててもあんま意味ないなと思ったので消した感じだ。

前回	今回

以前のSPビューでの結果	今回のSPビューでの結果

以前のPCビューでの結果	今回のPCビューでの結果

改善点としては、以前リンクの隙間が狭いといわれていたので微妙に広げてみたくらいで、後はほとんど特に何もしていないが、100点になってしまった。シングルカラムになったからかもしれない。前回の結果を保存していないので詳細は分からないけど、100点になったのでいっかという感じ。

例えば以下のようなコード（例示用のコードなので動作的な意味は特にない）

<>
  {isHoge && obj?.payload?.username !== undefined && (
    <div>新規：{obj.payload.username}</div>
  )}
  {isPiyo && obj?.payload?.username !== undefined && (
    <div>更新：{obj.payload.username}</div>
  )}
  {isFuga && obj?.payload?.acceptTerm !== undefined && (
    <div>退会：{obj.payload.acceptTerm}</div>
  )}
</>

ifやswich文で分岐処理を書く場合はIntelliSenseで漏れが分かるが、JSXに埋め込んで書くと書き漏れがあった場合に気づきづらい。また上記程度ならまだいいと思うが、ネストされていたり、三項演算子を使った複雑な分岐があったりすると読みづらく、何が起きるのかわかりづらいし、テストも書きづらいので避けたほうが良いと感じる。

このような場合はifやswich文で分岐してJSXを返すコンポーネントを別に作り、それを埋め込んだ方が良いと思う。ページ用のtsxファイルの中にこの手のものが大量にいると、大変見通しが悪いと感じる。

そもそも上記の条件はisHoge, isPiyo, isFugaの関連性が分からないため、MECEでなく、条件分岐としてもよくない。

Next.jsなどの画面をはじめとしたプログラミング設計において、状態管理は非常に重要な要素だ。特に、複雑なアプリケーションでは、状態がどこに存在するかがコードの可読性や保守性に大きな影響を与える。

基本的に状態は親にすべて望むのが望ましい。これは子が状態を持っている場合に、親がこれを利用することが困難だからだ。例えば以下のようなコンポーネント構造において、子コンポーネントAでAPIをコールし、その結果を持っているとする。仕様変更でこのデータを子コンポーネントBでも利用したいとなった場合、子コンポーネントAから状態を引きはがす必要があるが、実装によっては状態と実装が密結合になっており、特にスパゲッティ化しているコードでは容易に引きはがせないこともあるだろう。

親コンポーネント
 ├子コンポーネントA
 └子コンポーネントB

しかし親であらかじめ状態を持っておけば、このような苦労は不要になる。子AでAPIをコールした結果を親で持っておけば、それをそのままBに引き渡せばいいだけだからだ。

これは信頼できる唯一の情報源にも通じる話で、情報源が分散していると誤って同じ状態を二つ持ってしまったり、実装が複雑化してしまうことがある。酷い場合では子コンポーネントAと親の両方でAPIをコールするような実装もあるだろう。APIコールのタイミングがずれることで期待した通りの結果が得られなかったり、通信が増えることでロードが増えUXを毀損したり、APサーバーの負荷が高まる懸念もある。

基本的に同じ責務を持つ実装はDRYであることが望ましいので、このような悲劇を生まないためにも状態は親で持っておくことが重要だと考える。

逆に責務の範囲に閉じることができるのであれば、子で持ってよいケースもある。例えば親画面から子画面を出すときに、子画面だけに状態をもっていたいケースがある。こういう場合には子に一時的な状態を持つのはありだろう。例えば以下のような保存ボタンを押したときにモーダルの内容を確定するようなものであれば、モーダル内の状態として持ってよい。これは親で使わないからだ。その代わり画面状態の初期化時は親から状態を渡してやり、保存ボタンを押したときはコールバックで通知するという構造だ。

type HogeModalProps = {
  shown: boolean;
  hoge: string;
  onClose: () => void;
  onApplyClose: (state: { hoge: string }) => void;
};

export const HogeModal = (props: HogeModalProps) => {
  const [hoge, setHoge] = useState(props.hoge);

  const shownStyle = props.shown ? { display: 'block' } : { display: 'hidden' };

  const updateHoge = (ev: FormEvent<HTMLInputElement>) => {
    setHoge(ev.currentTarget.value);
  };

  const closeWithApply = () => {
    props.onApplyClose({ hoge });
  };

  return (
    <dialog style={shownStyle}>
      <input type="text" value={hoge} onInput={updateHoge} />
      <button onClick={closeWithApply}>保存</button>
      <button onClick={props.onClose}>キャンセル</button>
    </dialog>
  );
};

このようなケースでは、子の状態を親で管理すると煩雑になるため子に持たせておいた方が良い。

普段でWeb系のコードを書いているときに、こういうのがよいのではないか？と考えていることを雑に書いてゆく。いわゆるクラスを使わず関数を主体にして実装するなんちゃって関数型モデルみたいなやつの話。雑に書いているのでところどころ変なかもしれない。コードはNext.jsを題材にして書いている。

端的に言うとSOLIDを軸に、適切にDRY、KISS、YAGNIといった一般的な設計原則を織り込み、MVCのように役割分担のあるアーキテクチャを利用することや、実装方式を統一することを意識している。

• 実装方式の統一
• MVCのように役割分担のあるアーキテクチャ
• SOLIDを意識する
  • 単一責任の原則
  • 開放閉鎖の原則
  • 依存性逆転の原則
• DRYについて
• KISSについて
• YAGNIについて
• 処理の境界を挟んだ時に影響が波及しないようにする
• あとがき

実装方式の統一

実装方式、コーディングスタイルを統一することで、微妙な書き方の差異に起因する不具合や、レビュー時のコスト、新人が来たときの迷いを減らせる。恐らくこれこそが最も重要な設計原則だと思う。

例えば以下のコードは書き方は違うがやりたいことは似ている処理群だ。もしチームに新人が来た場合、新人はどちらを書けばよいだろうか？レビューの時に複数の実装パターンが混ざったときにレビュアーはどう対応すればいいだろうか？

// undefinedの判定方法の違い。よほど特殊なことをしているのでなければ、基本的に後者でいい
if (typeof hoge === 'undefined') { }
if (hoge === undefined) { }

// 関数の定義方法の違い。どちらかに統一するほうが望ましい
function func1(param: T) { }
const func1 = (param: T) => {}

// 仮引数の定義方法の違い。これは分割代入とそれ以外で致命的に振る舞いが変わることがあるので統一することが望ましい
const func2 = ({ hoge, piyo }: { hoge: string, piyo: number }) => {}
const func2 = (param: { hoge: string, piyo: number }) => {}
type Func2Param = {
  hoge: string;
  piyo: number;
}
const func2 = (param: Func2Param) => {}

何よりこれらの処理は同じような処理に見えて、微妙に振る舞いが変わるものがある。それを意識せずコードスタイルとして表現していた場合に不具合を引き起こすトリガーになりやすい。特に分割代入はオブジェクトの参照が切れるので予期せぬ動作になりやすいし、関数のスコープが長くなってきたときに由来が解りづらくなるケースがある。逆に分割代入をしないことによって変数名が冗長になることもある。

こういった問題を解決するために、コードの書き方をあらかじめチームで決めておくのがよい。特にチーム開発でコードの書き方に秩序がないプロジェクトは品質が低い傾向があると感じる。例えば車のタイヤを全て別のメーカーにしていても車は走行できるが、基本的にメーカーが違う場合、品質などの差異があるので、追々予期せぬ問題が発生する可能性がある。それと同じ問題がコードにもあると私は考えている。

他にも同じことがしたいのに複数の実装方法があるとgrepしづらかったり、直感的ではないなど、コードを読んだ人間の認知負荷の増大に繋がるため、基本的には統一されていたほうがよいだろう。

MVCのように役割分担のあるアーキテクチャ

私が普段考えているレイヤリングはMVCとClean Architectureを足して二で割ったようなものだ。DDDや厳密なClean Arctectureだと複雑で重くなりがちなので、このくらいが丁度いいのではないかと考えている。これでも十分複雑なのは承知している。

SOLIDを意識する

リスコフの置換原則と、インターフェース分離の原則については、今回のケースでは無視でいいと思っている。

単一責任の原則

一つの関数には一つの責務だけを持たせようという原則だ。

一例としてReactでよくみられるreducerは以下のように書ける。これはreducerはaction.typeに応じた関数を呼び出すことを責務として、実際の処理は関数に移譲するといった内容だ。

const reducer = (state, action) => {
  switch (action.type) {
    case 'update_account':
      updateAccountProc(state, action.payload);
    break;
      ...
  }
}

この実装であればreducer関数のテストは呼び出す関数をすべてモックしておき、action.typeに応じた関数が、想定通りの引数で呼ばれているかどうかを確認するだけでいい。各関数は関数単体でテストが書けるのでシンプルだ。

もしこれが関数呼び出しではなく、処理がベタで書いてあると単体の規模が大きくなり、コードの見通しが悪くなり、比例してテストコードも長くなり、個人的にはあまりよくないと思っている。コードの見通しは良いほうがよいし、責務ごとに関数を切ることでコードマージ時のコンフリクトの規模を抑えられることや、Feature flagを導入しやすくなるなど、利点は多いと思う。

欠点としては細切れになることによって、パッと見何をしているかわかりづらくなることがあると思うが、適切に抽象化し、関数名をちゃんとつけていれば基本的にそこは考慮しなくてよいと考えている。

開放閉鎖の原則

この原則は拡張に対しては開かれており、修正に対しては閉じられていないといけないということだ。

例えば以下のコンポーネントではUsernameという単語がAccountNameに代わったときにPropsを書き換える必要があるが、もしもuserNameというワードを使わずにonChangeやvalueといった抽象的な用語にしておけば、変更を不要にできる。

type Props = {
  onUserNameChanged: (username: string) => void;
  userName: string;
}

export const UserNameInputField = (props: Props) => {
  const onChange = (event: React.ChangeEvent<HTMLInputElement>) => {
    props.onUserNameChanged(event.target.value);
  };

  return <input type="text" onChange={onChange} value={props.userName} />;
}

多くのライブラリやAPIではメソッド名やプロパティ名は汎用的な名称になっていることが多く、アップデートで名前が破壊されることはそこまで多くない。こういった風に実装しておくことで内部実装への影響なく処理ができるという寸法だ。

依存性逆転の原則

これは下位コンポーネントに実装を持たせず、上位コンポーネントから中小を介して実装を注入する原則だ。

例えば以下のような実装がある場合、「この辺りに長い色んな処理があるものとする」の部分を仕様変更などで書き換えないといけなくなることがある。

type Props = {
  onChange: (value: string) => void;
  value: string;
}

export const UserNameInputField = (props: Props) => {
  const onChange = (event: React.ChangeEvent<HTMLInputElement>) => {
    /**
     * この辺りに長い色んな処理があるものとする
    */
    props.onChange(event.target.value);
    /**
     * この辺りに長い色んな処理があるものとする
    */
  };

  return <input type="text" onChange={onChange} value={props.value} />;
}

しかし、props.onChangeの中に前後の処理を挟んでいれば、これを回避することができるし、onChangeの中に処理がうじゃうじゃあるのも責務として過剰だと思うので、親からonChangeに対し何かしらの処理をすることが自明な関数を注入することで、こういった問題を回避できる。

DRYについて

この原則については、基本的に過度に意識する必要はなくシステム全体の共通部品以外は二重実装を許容して構わないと考えている。

これは余りにもDRYにしすぎると、仕様変更などで影響箇所が広がりすぎて修正が困難になるからというのと、共通部品が多すぎると探すのが大変で、最悪自力で実装されるケースもあるためだ。

KISSについて

程度問題ではあるが、DDDのような冗長で難解な設計原則は避け、ある程度単純なものにしたほうがいいというのは思っている。

べた書きをすることでコードジャンプが減って見通しがいいという意見は行き過ぎだと考えている。

YAGNIについて

これは技術選定フェーズで主に使うもので、何かの技術を入れるとき、その技術である必要がなければ使わないのがよいと考えている。

例えばWeb画面からAPIを叩くときにGraphQLを採用するケースがよくあると思うが、RESTやWebAPIで済むならそちらのほうがよい可能性がある。

GraphQLはWeb標準ではなく、ライブラリによって実装もまちまちで、複雑な仕組みが絡み合っており、全容を理解するのはなかなか難しいうえに、HTTPの上にあるため、使いこなすためには基礎の理解が必要だ。スキーマ設計も難しく、単純にやるとWebAPIと特に変わらないものができてしまう。そう考えたときに、絶対にGraphQLでないといけないというケースがないのであれば、より簡単な技術を使ったほうがよいと思っている。

複雑なものを作っている時間で休憩でもしたほうが頭がすっきりするだろう。

処理の境界を挟んだ時に影響が波及しないようにする

これはAPI通信など、外部から貰ってきたデータがあったときに、内部向けにデータ変換を行うということだ。

例えばAPIからisHoge, isPiyo, isFugaというデータが来るパターンがあり、この三つのフィールドはいずれか一つしかtrueにならないケースがあるとして、画面ではこれに対応するラジオボタンを持っている場合、trueだった項目を文字列として持っておくと便利だ。こういった場合に、使う場所で毎回変換処理を挟むと将来的に修正漏れが起きるリスクが増えたり、個々の場所で微妙に違う実装になって実装方式に統一がなくなったり。そもそも出現箇所が増えて認知負荷が増えるなど厄介だ。

これを防ぐために、一番最初にレスポンスを受けた時点で変換処理を挟み、以後それを引き回すというのがよいと考えている。APIに戻すときはisHoge, isPiyo, isFugaのフォーマットに戻す必要があるのが、その場合はAPIを叩く前に逆変換の処理を挟むと、データ変換処理が入り口と出口だけに存在することになり、データフローがシンプルになる。

他にもAPIと画面の境界でENUMの内容を変換する処理を入れておくとAPI側がしれっとリファクタなどで名称変更をしたときに、異常値を検出しやすくなるし、画面の開発者は基本的にAPIのことを考えなくてもよくなるので楽になると思っている（境界部分を設計する人間だけが考えればよくなる）

あとがき

雑に書こうとしたものの、結局まとまりきらず、まとめようとしても永遠に終わらなかったので一旦吐き出してみた。多分全部話そうとすると発散しすぎて収拾がつかなくなるので、個々について深ぼったことを書いて、最後にそれをまとめたほうがいいのかもしれない。

といっても何を書くかが問題になってくるので、日々思ったことを雑にアウトプットしつつ、あとで振り返ってまとめていくのがいいのかもしれない。