(息抜きコーディング)条件分岐をごにょごにょ。それとユニットテスト。

昨日は if の書き方で息抜きコーディングしたんだけど

今日もこんなツイートを見かけたので息抜きをすることにした。if が流行ってるのかな?

元のコード

(関数名は変えちゃった)

export function type1(prevX, prevY, nextX, nextY) {
  const toLeft = prevX - nextX > 0;
  const toRight = prevX - nextX < 0;
  const toTop = prevY - nextY > 0;
  const toBottom = prevY - nextY < 0;

  return {
    horizontal: toLeft ? 'left' : toRight ? 'right' : 'neutral',
    vertical: toTop ? 'top' : toBottom ? 'bottom' : 'neutral',
  };
}

export function type2(prevX, prevY, nextX, nextY) {
  return {
    horizontal: prevX - nextX > 0 ? 'left' : prevX - nextX < 0 ? 'right' : 'neutral',
    vertical: prevY - nextY > 0 ? 'top' : prevY - nextY < 0 ? 'bottom' : 'neutral',
  };
}

export function type3(prevX, prevY, nextX, nextY) {
  let horizontal = 'neutral';
  if (prevX - nextX > 0) {
    horizontal = 'left';
  }
  if (prevX - nextX < 0) {
    horizontal = 'right';
  }

  let vertical = 'neutral';
  if (prevY - nextY > 0) {
    vertical = 'top';
  }
  if (prevY - nextY < 0) {
    vertical = 'bottom';
  }

  return { horizontal, vertical };
}

おもしろいね

感想

もし、こういうコードのプルリクエストが来たら、自分の好みはあるからそれは伝えるけど、「これが自分にとってのベストなんだー!」って理由を説明してくれるなら、別にどれでもいいかなという気持ち

で、自分の好みは?

僕だったら、こうかくかなぁ。昨日の記事に影響受けてる!

export function type3_3(prevX, prevY, nextX, nextY) {
  const horizontal = (() => {
    if (prevX > nextX) return 'left';
    if (prevX < nextX) return 'right';
    return 'neutral';
  })();

  const vertical = (() => {
    if (prevY > nextY) return 'top';
    if (prevY < nextY) return 'bottom';
    return 'neutral';
  })();

  return { horizontal, vertical };
}

メインのコードの話はここでおしまい

ところでUT

今日のお昼にブロッコリーさんの記事をみて、なるほどー。ってなったので、今日は Jest を入れて UT を書くところから始めてみた。というか UT をあーだこーだするのに何時間か使って、実際のコードのリファクタリングは5分くらいで終わってしまったw

その1:最初に書いた UT

Xが左から右に、Yが上から下に増えていく座標系なのねって思いながら、とりあえずパラメタライズドテストでやってみるかーって書いたもの:

import * as targetFunctions from './detectMoveDirection';

describe.each(Object.values(targetFunctions))('%p test', (targetFunction) => {
  test.each`
    prevX | prevY | nextX | nextY | expected
    ${20} | ${20} | ${30} | ${30} | ${{ horizontal: 'right', vertical: 'bottom' }}
    ${20} | ${20} | ${10} | ${30} | ${{ horizontal: 'left', vertical: 'bottom' }}
    ${20} | ${20} | ${20} | ${30} | ${{ horizontal: 'neutral', vertical: 'bottom' }}
    ${20} | ${20} | ${30} | ${10} | ${{ horizontal: 'right', vertical: 'top' }}
    ${20} | ${20} | ${10} | ${10} | ${{ horizontal: 'left', vertical: 'top' }}
    ${20} | ${20} | ${20} | ${10} | ${{ horizontal: 'neutral', vertical: 'top' }}
    ${20} | ${20} | ${30} | ${20} | ${{ horizontal: 'right', vertical: 'neutral' }}
    ${20} | ${20} | ${10} | ${20} | ${{ horizontal: 'left', vertical: 'neutral' }}
    ${20} | ${20} | ${20} | ${20} | ${{ horizontal: 'neutral', vertical: 'neutral' }}
  `('($prevX, $prevY) -> ($nextX, $nextY)', ({ prevX, prevY, nextX, nextY, expected }) => {
    expect(targetFunction(prevX, prevY, nextX, nextY)).toStrictEqual(expected);
  });
});

targetFunctions のところは気にしなくて大丈夫。detectMoveDirection.js ファイルに新しい関数を書いたらそれも全部同じテストを通るようにしただけで、今日の記事のためだけのものなので、これ以降の例では省略する

例えば WebStorm で実行するとこんな感じになる

これくらいの関数だったら、別にこのくらいのテストでもいいかなって思う。んだけど、色々遊んでみた

そうそう、僕はあんまり期待値をテストメソッド名に書かない人です。条件だけ書くことが多い(現在のところ)

その2:ちょっと変えてみる

要らなさそうな情報があるから削ってみようかな

  • prevXprevY は変わらないので、パラメータから除外して値を直接埋め込んでみる
  • expected に Object じゃなくてそれぞれの値を書いてみる

するとこうなった

  test.each`
    nextX | nextY | expectedHorizontal | expectedVertical
    ${30} | ${30} | ${'right'}         | ${'bottom'}
    ${10} | ${30} | ${'left'}          | ${'bottom'}
    ${20} | ${30} | ${'neutral'}       | ${'bottom'}
    ${30} | ${10} | ${'right'}         | ${'top'}
    ${10} | ${10} | ${'left'}          | ${'top'}
    ${20} | ${10} | ${'neutral'}       | ${'top'}
    ${30} | ${20} | ${'right'}         | ${'neutral'}
    ${10} | ${20} | ${'left'}          | ${'neutral'}
    ${20} | ${20} | ${'neutral'}       | ${'neutral'}
  `('(20, 20) -> ($nextX, $nextY)', ({ nextX, nextY, expectedHorizontal, expectedVertical }) => {
    expect(targetFunction(20, 20, nextX, nextY)).toStrictEqual({
      horizontal: expectedHorizontal,
      vertical: expectedVertical,
    });
  });

んー。まぁ悪くはない・・・prevXprevY をなくしたからスッキリはしたものの、どんな値を使ってるの?って一瞬探してしまうから、そんなに変わんないか。どっちかっていうと最初の方が好きかも

その4:意図を書いてみる

今度は意図を書いてみよう:

  describe('Xが増加', () => {
    test('Yが増加', () => {
      expect(targetFunction(20, 20, 30, 30)).toStrictEqual({
        horizontal: 'right',
        vertical: 'bottom',
      });
    });
    test('Yが減少', () => {
      expect(targetFunction(20, 20, 30, 10)).toStrictEqual({
        horizontal: 'right',
        vertical: 'top',
      });
    });
    test('Yが変わらない', () => {
      expect(targetFunction(20, 20, 30, 20)).toStrictEqual({
        horizontal: 'right',
        vertical: 'neutral',
      });
    });
  });
  describe('Xが減少', () => {
    test('Yが増加', () => {
      expect(targetFunction(20, 20, 10, 30)).toStrictEqual({
        horizontal: 'left',
        vertical: 'bottom',
      });
    });
    test('Yが減少', () => {
      expect(targetFunction(20, 20, 10, 10)).toStrictEqual({
        horizontal: 'left',
        vertical: 'top',
      });
    });
    test('Yが変わらない', () => {
      expect(targetFunction(20, 20, 10, 20)).toStrictEqual({
        horizontal: 'left',
        vertical: 'neutral',
      });
    });
  });
  describe('Xが変わらない', () => {
    test('Yが増加', () => {
      expect(targetFunction(20, 20, 20, 30)).toStrictEqual({
        horizontal: 'neutral',
        vertical: 'bottom',
      });
    });
    test('Yが減少', () => {
      expect(targetFunction(20, 20, 20, 10)).toStrictEqual({
        horizontal: 'neutral',
        vertical: 'top',
      });
    });
    test('Yが変わらない', () => {
      expect(targetFunction(20, 20, 20, 20)).toStrictEqual({
        horizontal: 'neutral',
        vertical: 'neutral',
      });
    });
  });

んー。長くなったなぁwでも意図は分かるのはとても良い。実行結果はこうなる

実は、↑の前に、こんなパラメーター化をしてみた↓んだけど、あんまり好きじゃなかったので却下した。これが「その3」

  describe.each`
    caseName           | nextX | expectedHorizontal
    ${'Xが増加'}       | ${30} | ${'right'}
    ${'Xが減少'}       | ${10} | ${'left'}
    ${'Xが変わらない'} | ${20} | ${'neutral'}
  `('$caseName', ({ nextX, expectedHorizontal }) => {
    test.each`
      caseNameY          | nextY | expectedVertical
      ${'Yが増加'}       | ${30} | ${'bottom'}
      ${'Yが減少'}       | ${10} | ${'top'}
      ${'Yが変わらない'} | ${20} | ${'neutral'}
    `('$caseNameY', ({ nextY, expectedVertical }) => {
      expect(targetFunction(20, 20, nextX, nextY)).toStrictEqual({
        horizontal: expectedHorizontal,
        vertical: expectedVertical,
      });
    });
  });

その5:それぞれのケースでいくつかの値をチェック

最初のテストだとあんまり気にならなかったけど、意図を書くと「あぁ、それぞれの意図の中にいくつかのケースを書きたいなぁ」って気持ちになった。ゼロ周りとか、マイナスとか。数値の最大最小値周りは…今日はいいや。みたいなことを考えてこう書いてみた:

  describe.each`
    prevX  | nextX
    ${10}  | ${15}
    ${0}   | ${1}
    ${-1}  | ${0}
    ${-15} | ${-10}
  `('Xが増加($prevX -> $nextX)', ({ prevX, nextX }) => {
    test.each`
      prevY  | nextY
      ${10}  | ${15}
      ${0}   | ${1}
      ${-1}  | ${0}
      ${-15} | ${-10}
    `('Yが増加($prevY -> $nextY)', ({ prevY, nextY }) => {
      expect(targetFunction(prevX, prevY, nextX, nextY)).toStrictEqual({
        horizontal: 'right',
        vertical: 'bottom',
      });
    });
    test.each`
      prevY  | nextY
      ${15}  | ${10}
      ${1}   | ${0}
      ${0}   | ${-1}
      ${-10} | ${-15}
    `('Yが減少($prevY -> $nextY)', ({ prevY, nextY }) => {

...(省略)

んー。色んなパターンがテストできていいけど、読みやすくはないかぁ

その6:もうちょっと読みやすくしてみた

X と Y のパターンを掛け合わせなくてもいいかなぁって思ったので、 組み合わせごとに describe を書いてみた。そして、これでいいかーって気持ちになった

  describe('Xが増加・Yが増加', () => {
    test.each`
      prevX  | prevY  | nextX  | nextY  | expectedHorizontal | expectedVertical
      ${20}  | ${20}  | ${30}  | ${30}  | ${'right'}         | ${'bottom'}
      ${0}   | ${0}   | ${1}   | ${1}   | ${'right'}         | ${'bottom'}
      ${-1}  | ${-1}  | ${0}   | ${0}   | ${'right'}         | ${'bottom'}
      ${-20} | ${-20} | ${-10} | ${-10} | ${'right'}         | ${'bottom'}
    `(
      '($prevX, $prevY) -> ($nextX, $nextY)',
      ({ prevX, prevY, nextX, nextY, expectedHorizontal, expectedVertical }) => {
        expect(targetFunction(prevX, prevY, nextX, nextY)).toStrictEqual({
          horizontal: expectedHorizontal,
          vertical: expectedVertical,
        });
      }
    );
  });

  describe('Xが増加・Yが減少', () => {
    test.each`
      prevX  | prevY  | nextX  | nextY  | expectedHorizontal | expectedVertical
      ${20}  | ${30}  | ${30}  | ${20}  | ${'right'}         | ${'top'}
      ${0}   | ${1}   | ${1}   | ${0}   | ${'right'}         | ${'top'}
      ${-1}  | ${0}   | ${0}   | ${-1}  | ${'right'}         | ${'top'}
      ${-20} | ${-10} | ${-10} | ${-20} | ${'right'}         | ${'top'}
    `(
      '($prevX, $prevY) -> ($nextX, $nextY)',
      ({ prevX, prevY, nextX, nextY, expectedHorizontal, expectedVertical }) => {
        expect(targetFunction(prevX, prevY, nextX, nextY)).toStrictEqual({
          horizontal: expectedHorizontal,
          vertical: expectedVertical,
        });
      }
    );
  });

  describe('Xが増加・Yが変わらない', () => {
    test.each`

...(省略)

実行結果

これでいっかー!

テストを色々書いて楽しんだので

あとは jest で watch オプションをつけてプロダクションコードをリファクタリングするだけだった。watch オプションをつけておくと、ファイルを更新するたびに関係するテストが再実行されるので便利。いっかいコードを書き間違えてテストが落ちたので、おぉ!って思いながら修正したのだ

おもしろかったー!

今回みたいなシンプルな関数に対するテストでは、「その1」で十分だと思う。もうちょっと条件とかが複雑で細かく挙動をチェックしておきたい場合は、「その6」にしたい気持ち

こういうときに、QAエンジニアがいてくれると「どんなケースを残しておくといいのかな?」って相談できて良いんだろうなぁと思うのだった

コードはここに置いといた: https://github.com/bufferings/detect-move-direction-js

おわりー