My Strange Code#3 責務の分離と例外処理(JavaScript)

自分が過去に書いたコードを見返してみておかしなコードを直してみます。

今回のコードは以下です。

const isRetailDept = (list, deptID) => {
  try {
    let targetDept = list.find(item => Number(item.id) === Number(deptID))
    if (targetDept.dept_section === '2') {
      return true
    } else {
      return false
    }
  }catch(e){
    console.error(e)
    return false
  }
}

一見正しく動いていそうです。

関数に複数の処理が入っている

この関数には二つの処理が含まれています。

  • IDを元に部門を取得する
  • 部門が小売かどうか判定する

特に一つ目の処理については、他のところでも使いそうな汎用的なものです。

実際に、別の関数でも同じような処理をしているものがあり、コードが重複していました。

そのため、上記の二つをそれぞれ別の関数に分けて、責務を単一にすると良さそうです。

例外処理がおかしい

この例外処理の場合、try内で実行時エラーになったときもfalseが返ります。

つまり、部門が小売部門ではなかったというケースなのか、実行時のエラーが発生したのかがわからなくなります。

そして、想定している実行時のエラーがなんなのかもあやふやです。

そのため、以下のようにすると良さそうです。

  • 指定したIDの部門が小売の場合はtrueを返す
  • 指定したIDの部門が小売ではない場合はfalseを返す
  • 指定したIDの部門がない場合は、エラーをthrowする

マジックナンバーがつかわれている

targetDept.dept_section === '2’ の '2’ が何を指すのかは実装者しかわかりません。関数名をみればわかりますが、ここは定数化して明示的にしたいです。

また、定数化することで、他のコードで小売を指す場合に使うことができ、仮にコードが2から3に変わった場合も定数だけ直せば問題ありません。

変更後

まだ改善の余地はありますが、以下のようにしました。

const RETAIL_DEPT_CODE = '2'
const NOT_FOUND_DEPT_ERROR = '部門が見つかりません'

const getDept = (depts, id) => {
  const result = depts.find(dept => dept.id === id)
  if(result === undefined) throw Error(NOT_FOUND_DEPT_ERROR)
  return result
}

const isRetailDept = (dept) => {
  return dept.deptSection === RETAIL_DEPT_CODE
}

isRetailDept(getDept(depts, 1) //こんな使い方

責務を分離することで、getDept は他のところでも使えるような汎用的なものになりました。

また、この関数を使うことで、部門が見つからない場合は例外を投げるという動作を強制するので、動作を統一することができます。

小売コードやエラーメッセージを定数化することで可読性が上がり、コードやメッセージを変えたい場合もこの定数の値を変えるだけで良いので、変更漏れを防ぐことができます。

JavaScript, プログラミング入門

Posted by たみぼうず