JavaScriptのコードには、こんな書き方もある

2022/05/07に公開
24

この記事はコードの書き方について、書き方そのものを推奨するものではなく、このような書き方も出来るという紹介です。コメント欄まで一緒にみていただくと学びになります。

※記事はいただいたコメントを反映しましたので、当時のコメントと記事の内容に差分があります

1.破壊的メソッドを避ける

破壊的メソッドとは、元の配列の要素を変えるメソッドです。以下の例ではconstで宣言した変数numbersが、pushメソッドにより更新されています。

対応前

const numbers = [1, 2, 3]
numbers.push(4)
console.log(numbers) // [1, 2, 3, 4]

この場合、元の配列の要素を更新するのではなく、スプレッド構文を使って新しい変数に代入します。変数はなるべくイミュータブルにしておくと、意図しない不具合やプログラムの可読性や保守性が向上します。pushメソッド以外にも、sortやfillなど他の破壊的メソッドでも使えます。

対応後

const numbers = [1, 2, 3]
const newNumbers = [...numbers, 4]
console.log(newNumbers) // [1, 2, 3, 4]

ただし、破壊的メソッドを使った方がメモリを節約できるため、パフォーマンスを重要視する場面では、有用ではありません。

2.配列の要素の存在チェックに比較演算子を使わない

配列の要素の存在チェックに、比較演算子を使うコードをよく見かけます。

const numbers1 = [1, 2, 3]
if (numbers1.length >= 1) {
    console.log('OK1') // 実行される
}

const numbers2 = []
if (numbers2.length == 0) {
    console.log('OK2') // 実行される
}

以下のように書き換えることが出来ます。要素がないことを表す場合、論理否定(!)を使うだけなので便利です。(参考: idiomatic.jsの4.条件文の評価)

対応前

const numbers1 = [1, 2, 3]
if (numbers1.length) {
    console.log('OK1') // 実行される
}

const numbers2 = []
if (!numbers2.length) {
    console.log('OK2') // 実行される
}

しかし、lengthはあいまいな比較になり、配列ではなく文字列などが渡された場合で挙動が変わるため、厳密等価演算子(===)を常に使用すべきです。

対応後

const numbers1 = [1, 2, 3]
if (numbers1.length >== 1) {
    console.log('OK1') // 実行される
}

const numbers2 = []
if (numbers2.length === 0) {
    console.log('OK2') // 実行される
}

3.オブジェクトの特定のプロパティを代入するときは分割代入を使う

オブジェクトのプロパティを取得する場合、2行目のように書いているケースをよく見ます。

対応前

const fruit = { id: 1, type: 'apple', price: 300 }
const type = fruit.type
const price = fruit.price
console.log(type, price) // apple 300

取得したいオブジェクトのプロパティ名を{}の中に指定することで、同じことができます。

対応後

const fruit = { id: 1, type: 'apple', price: 300 }
const { type, price } = fruit
console.log(type, price) // apple 300

4.同じ処理はコールバック関数を利用して一つにまとめる

以下の処理は、membersとfruitsの配列でidの存在チェックを行い、id一覧の配列を生成します。

対応前

const members = [{id: 11, name: 'yamada'}, {id: 12, name: 'tanaka'}, {name: 'sasaki'}]
const fruits = [{name: 'orange'}, {id: 33, name: 'apple'}, {id: 34, name: 'banana'}]

const memberIds = members.filter(member => member.id).map(member => member.id)
const fruitIds = fruits.filter(fruit => fruit.id).map(fruit => fruit.id)

console.log(memberIds) // [ 11, 12 ]
console.log(fruitIds) // [ 33, 34 ]

よく見るとmembersとfruitsのidの取得は、メソッドチェインでfilterしてmapで要素を返しているだけです。同じような処理は、ひとつにまとめられます。

対応後

const members = [{id: 11, name: 'yamada'}, {id: 12, name: 'tanaka'}, {name: 'sasaki'}]
const groups = [{name: 'orange'}, {id: 33, name: 'apple'}, {id: 34, name: 'banana'}]

const getIds = (args) => args.filter(arg => arg.id).map(arg => arg.id)

const memberIds = getIds(members)
const fruitIds = getIds(groups)

console.log(memberIds) // [ 11, 12 ]
console.log(fruitIds) // [ 33, 34 ]

5.極端に数値が多いときは短くする書き方もある

const num = 100000000
console.log(num) // 100000000

JavaScriptでは、数値の間に文字列「e」を追加し、ゼロの数を指定することで数値を短くできます。しかし、1000程度の数値であれば、そのままの方が見やすいです。

const num = 1e8
console.log(num) // 100000000

数値はアンダーバーを入れることで区切ることも可能です。大きな数値に関しては、こちらの方が読みやすいです。

const num = 100_000_000
console.log(num) // 100000000

6.ifやswitchなどの条件分岐をなるべく使わない

const toColorCode = (value) => {
    let code = ''
    switch (value) {
        case 'blue':
            code = '#0000ff'
            break;
        case 'yellow':
            code = '#ffff00'
            break;
        case 'red':
            code = '#ff0000'
            break;
    }
    return code
}

console.log(toColorCode('yellow')) // #ffff00

以下ように書くとコード量が減って読みやすくなります。しかし、該当がない場合にundefinedになったり、万が一文字列のconstructortoStringが渡された時にfunction toString() { [native code] }が出力され、これをBool値に変換するとtrueになるため、この先の処理で予期せぬバグを生み出す可能性があります。

対応前

const toColorCode = (value) => {
    const color = {
        blue: '#0000ff',
        yellow: '#ffff00',
        red: '#ff0000'
    }
    return color[value]
}

console.log(toColorCode('yellow')) // #ffff00

この場合はMapを使うことが推奨されています。

良い

const color = new Map(
  [
    ["blue", "#0000ff"],
    ["yellow", "#ffff00"],
    ["red", "#ff0000"]
  ]
)
console.log(color.get('yellow')) // #ffff00

※サンプルコードでは予期せぬ値(空文字など)が渡される場合は、switch文の例と他の例とでは挙動が異なりますのでご注意ください。詳しくはコメント欄をご確認ください。

7.関数の引数が多い場合、Patterns Objectを利用する

createTask関数の引数が4つあります。関数の呼び出し元だけを見ると「特になし」や「false」などが何を表すのか、関数の定義を見にいかないと分からないです。(初期化処理の例が悪いですが、サンプルなので大目に見てください)

対応前

const createTask = (title, description, dueDate, isDone) => {
  // 何かしらの処理
}

createTask('勉強する', '特になし',  '2022-08-01', false)

関数の引数をオブジェクトで定義することにより、呼び出し元にプロパティ名をつけられます。このようにすると、呼び出し元を見たときに引数に必要な値が分かりやすいですし、プロパティの順番を間違えずに済みます。

対応後

const createTask = ({title, description, dueDate, isDone}) => {
  // 何かしらの処理
}

createTask({
    title: '勉強する',
    description: '特になし',
    dueDate: '2022-08-01',
    isDone: false
})

Discussion

standard softwarestandard software

1 bad good とはいえない。
破壊的メソッドの使用と不具合や可読性や保守性の向上には関係ないと思います。
なぜsortが非破壊的なものがないかというと、連続したsortでメモリが異常に増えるからでしょう。
破壊的の名前からして誤解させますが、破壊的=悪、とみなさない方がよいと思います。

2 bad と good が明確に逆
あいまい等価演算子を利用するのは基本的にbad
if (number1.length) は if (number.length == 0) と同じなのであまり望ましくなく
if (number1.length === 0) の方が堅実
配列に対するlengthだからいいけど、独自オブジェクトに対する.sizeとかになると途端に困った事になり不具合が増えます。

4 主題と関係ないけど
args.filter(arg => arg.id)
id = 0 (あるいは空文字)なら、idの存在判定ができなくなるので、bad。
args.filter(arg => arg.id !== undefined) などがよいかもしれない。

5 ?、1e4 とか、単に読みにくい気がします。

6 bad と good が明確に逆
toString とか、constructor という文字を渡せばわかる。

手厳しくて申し訳ないですが、明確な間違いが他の初心者プログラマに広がるのは望ましくないと思い書きました。

この手の間違った情報を紹介しているサイトが沢山あるので、たぶんあなたも誰か間違ったことを言っている人のテクニックを読んで理解したからこういうバッドノウハウを身に着けているんだと思いますが、ネットの情報を鵜呑みにせずに噛み砕いて学ぶべきに思います。

TCTC

ご指摘いただきまして、ありがとうございます。

1 bad good とはいえない。

仰るとおりメモリの観点がなかったです。状況によっては全て悪にはならないですね。
ありがとうございます。

可読性などに関しては、変数の値は基本的に不変にする、可変にする場合は別の変数を新たに用意することで、その変数は何者かを明確にする目的がありました。処理後にsortedXXXのような変数名を用意することでソート済みかどうか判断できるので分かりやすいと言った意図です。

2 bad と good が明確に逆

配列の要素数を数えるために、厳密等価演算子を使う理由ってなんでしょうか?lengthメソッドを使った時点で数値に変換されるため、わざわざ厳密にしなくて良いと判断していました。

6 bad と good が明確に逆

こちらの具体をもう少しお伺いできますか?イメージが出来ておらずでして。

standard softwarestandard software

1 参考

イミュータブル・イミュータブル言ってるのは優秀なプログラマーですか?に対する回答 - Quora
https://jp.quora.com/イミュータブル-イミュータブル言ってるのは優秀な/answers/322576960

2
配列なので、const a = [0,1,2]; に対して、if(a.length) で判定ができるのですが
プログラムのどこかで間違った値が設定された場合次のようになるので、見抜きにくい不具合になってしまいます。常に「===0」にしておけば、間違った値が入っていてもすぐ見抜けます。

length ならまだわかりやすいですが、sizeというメソッドの場合に、if(a.size) となっていると、わかりにくいということになります。なので、上記のテクニックを鵜呑みにしたまま、if (a.size === 0) を、if (!a.size) に変えてしまうのは、Badなパターンです。

const a = [];
if (!a.length) {
  console.log('!a.length:配列は空です'); // 呼び出される
}
if (a.length == 0) {
  console.log('a.length == 0:配列は空です'); // 呼び出される
}
if (a.length === 0) {
  console.log('a.length === 0:配列は空です'); // 呼び出される
}
const b = {  length: ``};
if (!b.length) {
  console.log('!b.length:配列は空です'); // 呼び出される
}
if (b.length == 0) {
  console.log('b.length == 0:配列は空です'); // 呼び出される
}
if (b.length === 0) {
  console.log('b.length === 0:配列は空です'); // 呼び出されない
}
const c = {  length: []};
if (!c.length) {
  console.log('!c.length:配列は空です'); // 呼び出されない
}
if (c.length == 0) {
  console.log('c.length == 0:配列は空です'); // 呼び出される
}
if (c.length === 0) {
  console.log('c.length === 0:配列は空です'); // 呼び出されない
}

参考
== null よりも ===null と === undefined を使おう - Qiita
https://qiita.com/standard-software/items/08597efad6dff1413897

6

const toColorCode1 = (value) => {
    let code = ''
    switch (value) {
        case 'blue':
            code = '#0000ff'
            break;
        case 'yellow':
            code = '#ffff00'
            break;
        case 'red':
            code = '#ff0000'
            break;
    }
    return code
}

console.log(toColorCode1('toString')); // ""

const toColorCode2 = (value) => {
    const color = {
        blue: '#0000ff',
        yellow: '#ffff00',
        red: '#ff0000'
    }
    return color[value]
}

console.log(toColorCode2('toString')); // function toString() { [native code] }

空文字以外で指定していない値が返ってくるのは予想外だと思います。

「6.ifやswitchなどの条件分岐をなるべく使わない」は、どこの誰が言ったのでしょう。ifやswitchは積極的に使うべきです。

参考
オブジェクトを連想配列として使うと痛い目にあいますよ
https://zenn.dev/standard_soft/articles/7458d1f49fd2ef

オブジェクト指向のプログラミングの問題点は何ですか?に対する回答 - Quora
https://jp.quora.com/オブジェクト指向のプログラミングの問題点は何で/answers/123036178
継承で自動的にメソッドが切り替わると単純にコードが読みにくくなることを書いている部分があります。

TCTC

確認しまして、理解しました。フィードバックいただいた内容を踏まえ、記事の内容はコードの書き方を推奨するのではなく、紹介程度に留めるよう修正しました。

ありがとうございます。

as8as8

「6.ifやswitchなどの条件分岐をなるべく使わない」は、どこの誰が言ったのでしょう。ifやswitchは積極的に使うべきです。

これは循環的複雑度として定義されてますね。
単に避けるべき、というのは少し極端かもしれませんが多用すべきというのは少なくとも静的解析の世界では複雑として扱われてますし、オブジェクト指向や関数型、リアクティブプログラミングなど多くのパラダイムでも否定されてるものかと思います。昨今の言語ではif/switch文はなく、if式やパターンマッチを取り入れる言語も増えてきてるのはこれらの表れでもあります。

イミュータブルについてもメモリまでほんとに気にしてJSを書く必要があるようなケースはそうそうない、あるならそれはそもそもJSという選択に再検討の余地があることの方が多い気がします(node.js以外や、クライアントサイドならwasmなど)。もちろん、一部そういうケースはありますが。
イミュータブルデータフローはfluxを皮切りにJS界隈では多く採用されてきたものなので、個人的には保守性や容易性においては有用に感じますし、少なくとも今のJS界隈においては否定できるものではないかと思います。

参考までに、toSortedなどのイミュータブルな操作メソッドもJSに追加が検討されています。
https://github.com/tc39/proposal-change-array-by-copy

standard softwarestandard software

これは循環的複雑度として定義されてますね。

循環的複雑度、という用語をはじめて聞いてWikiPediaを見ましたが、特に分岐コードを避けて他の手段で分岐させるということについてはあまり検討されている概念ではないと思いました。

switch や if で分岐するのは明示的でわかりやすいですが、分岐には見えないのに分岐されているとコードは循環的複雑度には現れない形で複雑さが増えるのでよりまずいと思います。

静的解析の世界では複雑として扱われてますし

静的解析の限界があるように思います。静的解析に現れないけど分岐しているなら、それは逆にやばいです。

オブジェクト指向や関数型、リアクティブプログラミングなど多くのパラダイムでも否定されてるものかと思います。

その説は聞いたことがないです。

オブジェクト指向の継承の問題(ポリモーフィズムの困難さ)については、長すぎる記事で申し訳ないですが、下記リンクで示してます。

オブジェクト指向のプログラミングの問題点は何ですか?に対する回答 - Quora
継承で自動的にメソッドが切り替わると単純にコードが読みにくくなることを書いている部分があります。

イミュータブルについてもメモリまでほんとに気にしてJSを書く必要があるようなケースはそうそうない
あるならそれはそもそもJSという選択に再検討の余地があることの方が多い気がします

普通にあると思います。

[イミュータブル・イミュータブル言ってるのは優秀なプログラマーですか?に対する回答 - Quora]
このリンク先でもユーザーリストに生年月日と誕生日の例を書いておきました。

また、表ソートの場合、
列クリックで、API呼び出してというのもありますが、API呼び出さずに、というのもあります。

そのときに別々の列を10回クリックして10回ソートさせたら、10回ディープコピーするのは苦しいです。自動でメモリ解放されるにしてもなんか遅くてやばくなるので、イミュータブルによって逆にやばい問題を引き起こす場合があります。

イミュータブルデータフローはfluxを皮切りにJS界隈では多く採用されてきたものなので

"イミュータブルデータフロー"も、初めてききましたが、JSを普通に使っているとイミュータブルとミュータブルを使い分けられなくて不具合におちいっているコードはよく見かけます。

※リンク先に下記のように書きました。

できるプログラマーの書くコードはイミュータブルであろうとミュータブルであろうと、
できないプログラマーより安全なコードになります。

どちらも理解して使いこなす事が大事だと思います。
ミュータブルだと問題になる場面だからここはイミュータブルにしようとすることがよくあるのは理解していますが、だからといってミュータブルが危険でイミュータブルは安全とはいいにくく、安全だからイミュータブルをずっと使うとなっても問題がおきそうなので、どちらも挙動を理解して使いこなすべきに思います。

as8as8

ifを減らし実装を意識させないためのポリモーフィズムの威力について誤解があるようなので解説したいところですが、不愉快な表現を含んでおりこれ以上の反論は本記事の筆者の方にに迷惑がかかりそうなのでやめておきます。

TCTC

asさんが良ければ、その話しに興味がありますので、是非お話しをお聞きしたいです。(少し時間が経ってしまったので、難しければ大丈夫です!)

as8as8

@TCさん
ポリモーフィズムはオブジェクト指向の概念で、誤解を恐れずに言えば「hogeというメソッドを実装してる何か」という風に抽象的なルールを設けることで、ifや知らなければいけないことが減りその方が有用とされることがある、という話でした。

わかりやすいか微妙かもしれませんが、例えば状況に応じて

  • シークレットブラウザならsessionStorageへ
  • テストの場合には自作のdummyStorageへ
  • そのほかはlocalStorage

という要件があったとします。それを愚直に書いてく場合と、setItemgetItemがどれにでも備わっている前提で書くコードではifの位置や数に違いが出てきます。

// before storageが増えたら2か所づつ修正しなければならない
if (IS_SECRET_BROWSER) {
  sessionStorage.setItem(KEY, 999)
} else if (IS_TEST_ENV) {
  dummyStorage.setItem(KEY, 999)
} else {
  localStorage.setItem(KEY, 999)
}
// ...何か長い処理とか
let result
if (IS_SECRET_BROWSER) {
  result = sessionStorage.getItem(KEY)
} else if (IS_TEST_ENV) {
  dummyStorage.getItem(KEY)
} else {
  result = localStorage.getItem(KEY)
}

// after どこに保存するかは1回だけ気にすれば良い
let storage
if (IS_SECRET_BROWSER) {
  storage = sessionStorage
} else if (IS_TEST_ENV) {
  storage = dummyStorage
} else {
  storage = localStorage
}
storage.setItem(KEY, 999)
// ...何か長い処理とか
const result = storage.getItem(KEY)

これは「storageという変数に入ってるものは、getItemというメソッドを持っている」という前提で成り立つコードです。こうすると長い処理が間にあったとしても、「session.getItem(KEY)でどっかのストレージから値が取得できる」ということだけ知ってればよく、対してbeforeは「どの環境のパターンがあるのか」「どの順番で分岐を書くべきなのか」などを常に考慮する必要があります。

こういった常に考慮すべきこと(頻出するifとか)というのを減らすことでバグを防ごうという取り組みで出てきた?有用とされてる概念?がいろいろあって、それがオブジェクト指向や関数型と呼ばれるものだったりします。ポリモーフィズムの場合はそれが「あるメソッドがある振る舞い(ここでいうとsetItemはどっかに保存してくれる)を持ってること」という特徴を共通化するアプローチでした。

求めてたような回答になってるか、少々雑な説明なのでわかりづらい点もあるかと思いますがご容赦ください。
また、以下の記事とかわかりやすいのでご参考までに。
https://qiita.com/Nossa/items/a93024e653ff939115c6

TCTC

私もif文などで条件分岐が多くなるコードに対しては、仰るとおりポリモーフィズムの検討や、早期リターンによるelseの削減を考えるようにしていたので、ご意見伺えて良かったです。

参考記事までいただき、ありがとうございます。

standard softwarestandard software

突っ込むのは申し訳ないところなのですが、どうしても見抜けてしまうので書いておきます。
(表現はおだやかに)

例示してある、afterの書き方は before と比べると仕様変更を
考えると破綻しそうな書き方なので、before の方が望ましいと私は思います。
細かくいうとbeforeの共通化の方法が少し違うということです。

storage の定義は基本アプリケーションの起動時とか特定の設定を切り替えたときとか行われ、
setItem / getItem は、ボタン押したときに行われるとかなので
storage の定義と、setItem とは、コード的に距離があります。

そうなると、

「特定条件では新しくクラウドストレージというのを使って。
  そのクラウドストレージの場合だけ、
  KEY="AAA"の場合だけ、
  読み取りは"CLOUD-READ-AAA"、
  書き込みは"CLOUD-WRITE-AAA"にしなきゃいけない
  制限があるんだよ。
  あ、クラウドストレージでも、BBBとか、CCCとかは、そのままでOKね。
  実装よろしくー」

という仕様変更がやってきたときに、(ありがちなヤツです)

そのボタンイベントハンドラをみつけて
storage.setItem(KEY, 999)
とか
storage.getItem(KEY)
とかみつけたときに、そこに分岐を書こうとすると
分散したところにストレージの分岐処理を入れようとして
どうやってもスパゲティコードを生み出してしまいます。

コードでより詳しく示します。

// before(を改良した)パターン

// 関数
const storageSetItem = (key, value) => {
  if (IS_SECRET_BROWSER) {
    sessionStorage.setItem(KEY, value)
  } else if (IS_TEST_ENV) {
    dummyStorage.setItem(KEY, value)
  } else {
    localStorage.setItem(KEY, value)
  }
}

const storageGetItem = (key) => {
  let result
  if (IS_SECRET_BROWSER) {
    return sessionStorage.getItem(KEY)
  } else if (IS_TEST_ENV) {
    return dummyStorage.getItem(KEY)
  } else {
    return localStorage.getItem(KEY)
  }
}

// ...
// ここの上下のコードには記載の位置に距離がある
// ...

// 関数呼び出し側
storageSetItem('AAA', 123);
const result = storageGetItem('AAA';

上記のbeforeだと、架空の仕様変更、
「クラウドストレージを追加して、KEY="AAA"の場合だけ、"CLOUD-READ-AAA"にする」
ときに、修正変更するのは簡単で、
変更容易な性質が高いです。

最近はやりの、関数型ってよぶ人もおおい書き方です。
昔は構造化プログラミングと呼ばれてましたね。

// afterのパターン

// 関数
const initSetting = () => {
  if (IS_SECRET_BROWSER) {
    storage = sessionStorage
  } else if (IS_TEST_ENV) {
    storage = dummyStorage
  } else {
    storage = localStorage
  }
};
let storage = initSetting();

// ...
// ここの上下のコードには記載の位置に距離がある
// ...

// 関数呼び出し側
storage.setItem(KEY, 999)
const result = storage.getItem(KEY)

上記のafterだと、架空の仕様変更、
「クラウドストレージを追加して、KEY="AAA"の場合だけ、"CLOUD-READ-AAA"にする」
に対応するのが相当むずかしくなります。

setItem / getItem の部分でかきかえざるおえなくなるので
initSetting でもストレージ分岐しているのに、
setItem/getItemでも分岐書くの?どうするの?
クラウドストレージのときだけ、setItem,getItemを上書きする?
となります。

setItem,getItemの上書きは上書き前の関数をバックアップして
新しい処理で上書きするのはぼちぼち腕いりますね。

なので比較的よいと考えられるのはシンプルに共通化して分岐している before の方です。

このような問題点があるために、
ここ数年ではポリモー的なオブジェクト指向はいけてない論が増えてきているように感じます。
(このコードはオブジェクト指向とは関係ないポリモーフィズムだけど)

また、beforeからより共通化したいなら下記のようになったりもします。

// before-after混在
const getStorage = () => {
  if (IS_SECRET_BROWSER) {
    return sessionStorage
  } else if (IS_TEST_ENV) {
    return dummyStorage
  } else {
    return localStorage
  }
};

const storageSetItem = (key, value) => {
  const storage = getStorage();
  storage.setItem(KEY, value);
}

const storageGetItem = (key) => {
  const storage = getStorage();
  return storage.getItem(key);
}

// ...
// ここの上下のコードには記載の位置に距離がある
// ...

// 関数呼び出し側
storageSetItem('AAA', 123);
const result = storageGetItem('AAA');

結局これも、架空の仕様変更
「クラウドストレージを追加して、KEY="AAA"の場合だけ、"CLOUD-READ-AAA"にする」
などがあると、
仕様変更を、storageSetItem や storageGetItem の内部で吸収できるので
after よりはましですが、before よりいまいち、ということになります。

afterのパターンで書かれたコードが満載されたプロジェクトでは
仕様変更仕様変更仕様変更の場合、
分岐を共通するところが分散されたコードが非常にスパゲッティになるので

ポリモーフィズムやダックタイピングで分岐を見えなくするやり方というのは
潜在的にスパゲティを引き起こす可能性が高いコードになり
余計な問題を引き起こしかねないので

beforeのように、実行したいその場で明示的に分岐をする。
隠蔽したいならその場で関数化して共通化する
という手法の方がスパゲティコードを生み出さないです。

また、beforeに対して、before-after混在にするのは、私は「やりすぎDRY」と読んでいて
不要なDRYによって変更容易制を壊してしまうやり方だと思っています。
そこまで共通化するべきではない場合がよくあるのです。

改めてまとめますが、
このような例の場合、上がよく下がよくない順で

  • 素直に考えるのはbefore、そしてこれで十分によい
  • beforeをさらに共通化したくなって、「やりすぎDRY」になってイマイチなのが、before-after混在
  • ポリモーフィズムやダックタイプみたいな変な概念を持ち込んで失敗するのが、after

となると思います。

このようなものを、ぐだぐだと長々説明しましたが
自分の中では見たらすぐ答えが出てしまいます。

わざわざ私の説明用に都合のよい仕様変更をいれたからこうなっていると思うかもしれませんが
コードみたらすぐに、将来の仕様変更等を何パターンも頭ですぐに想定して、
こういう可能性があるからこっちのほうがいいよね、と見抜けるかそうでないか、
というのが、プログラマの腕のひとつかなと思います。

「ポリモーフィズムを活用するとなぜ if や switch が消えるのか? - Qiita」を読みましたが、面積求めるのにif文を駆逐するとかありますが、駆逐する必要性などどこにもなく、
ifやswitchで分岐することが複雑になる、というのは全くそんな事はないので、正しくない概念が広がってしまっているなー、と思ったりします。

udyestudyest

辞書的な使い方をするならMapが使えます。
オブジェクトだと意図を汲み取りにくいですが、Mapなら辞書として使いたいということが自明なので、個人的にはこちらをよく使用します。

const colorMap = new Map([
  ['blue', '#0000ff'],
  ['yellow', '#ffff00'],
  ['red', '#ff0000']
]);

console.log(colorMap.get('blue')); // '#0000ff'
console.log(colorMap.get('green')); // undefined
udyestudyest

あ、standard softwareさんが紹介してくださっている記事でもMapについて触れられていましたね。二番煎じになってしまいました、失礼しました。

TCTC

コメントありがとうございます。
今後はMapも使ってみようと思います!

gaga

standard software さんも仰られているので多くは書きませんが、2 と 6 は good が 明確に bad なので修正を検討してはいかがでしょうか。

2 常に厳密比較演算子を使用してください。

6 そもそも bad と good で挙動が異なっています。両方のサンプルコードの toColorCode に空文字列を渡してみてください。

TCTC

ありがとうございます。記事を修正するとコメントと記事の内容が合わなくなり読みにくくなるため、敢えて修正していなかったのですが、コメント欄まで見ない方もいると良くないので先ほど修正しました。

両方のサンプルコードの toColorCode に空文字列を渡してみてください。

6については、gaさんの意図が読み取れませんでした。空文字かundefinedかの違いかと思いましたが、何か他にありましたか?

gaga

説明不足で申し訳ありません。6については次の2点が気になりました。

1
修正前では、例示のコードが記事の内容に対して不適当だと感じました。(主観)
私はこの記事を読んで、少なくとも bad と同じことが good でも出来ることを期待しました。(その上で good には何かしらのベネフィットがある…という期待)
なので、同じ入力に対して出力が異なるのはサンプルとして不適当なのでは、と思いました。

2
同じ入力に対して出力が異なるのはシンプルにバグの元になります。
例えば、元々 bad の実装をしていて、good に変えるとします。

bad の実装に#を切り取る機能を追加しました。

console.log(toColorCode('yellow').replace('#', ''))  // ffff00

bad の実装では空文字列を渡すと空文字列が返るので、当然 String に生えているメソッドも実行できます。

console.log(toColorCode('').replace('#', ''))  // 

リファクタリングを行い good のコードになった時、空文字列を渡すと undefined が返るのでエラーになります。

console.log(toColorCode('').replace('#', ''))  // TypeError: Cannot read property 'replace' of undefined

今回のユースケースではわざわざ空文字列やメソッド名の文字列を渡したりしないと思いますが、このような隙は作らないに越したことはないです。

TCTC

同じ結果を得られるかという点においては、確かに不適切でした。

元々goodとして挙げた背景は、ESLintのcomplexityのルールに反する既存コードがあった時に、ifやswitchに頼ることができず最終的に辿り着いた解であったため、このような解決策もあるという事を示したものでした。

サンプルコードに関しては、正直どこまで細かく対応すべきか悩んでおりますが、予期せぬ値(空文字など)がある場合は挙動が異なりますと注意文言を添えさせて頂くことに致します。※他に良い書き方があればぜひ参考にさせて頂きますので、仰って頂ければと思います。

具体的な事例まであげていただき、ありがとうございます。

yuki2006yuki2006

こんにちは!

2のところで 「参考: idiomatic.jsの4.条件文の評価」が参考に挙げられてますが、
このidiomatic.jsはよくわかってないのですが、

airbnbのstyle guideのほうがよく引用されているイメージです。

https://github.com/airbnb/javascript#comparison--shortcuts

TCTC

こんにちは!

airbnbのstyle guideのほうがよく引用されているイメージです。

仰るとおりですね。私もよく参考にしていました。idiomatic.jsについては、airbnbの他にも色々と参考になりそうな記事をGitHubで探していたら、たまたま見つけました。

Starがそれなりに付いているのと、リポジトリの更新期間を見てある程度メンテされていると思い、良さそうだったので参考にさせていただいた次第です。

standard softwarestandard software

もうちょっと書いておきます。くどくてすいません。

idoomatic の 4.1.x. で書かれていることには疑問を感じます。
もともと厳密等価を行っているのに、それを改悪している感じがあります。理由は例示したものやリンク先で示した通りです。

静的型言語的に、array や string や boolean に変数の型が確定している場合は
if (foo === '') を if (foo) とか書くのはそれほど問題ないのですが
JSは、TSではないので、動的型言語なんです。fooはstring確定してない事もあるんですよ。

また別な場面では
if (foo) で、数値の0を想定してコード書いていたのに
文字列の""が来てるのに、顕在化せずになんとなく動いてしまう、という場合もあります。
そんなコード書くなバカと、いいたくなるのですが、そんなコード書いてもどうにかみつけやすいのが
if (foo === 0) であって、if (foo) と書いてあると、見つけられない。

また別の場面で
if (foo) の部分をリファクタリングしたくなった場合
if (foo === 0) や if (foo === false) や if (foo === null) と書いてくれていれば
リファクタリングしやすいのですが、
if (foo) は、正確にどのようにリファクタリングすればいいのかがわからなくなり
コード書いた人が何の値を意図してその条件分岐を書いたのかわからなくなり、コードリードが苦しくなります。
0.1秒で理解できることが、数秒かけないと理解できないならそれは数十倍にコードリードが非効率です。

A:if (!array.length) を
B:if (array.length === 0) に書き直すべきだ、とかは思いません。
arrayが配列だとほぼ確定するので、そこまで冗長な書き方を強要は普通しない。
length を独自プロパティで持つアホなコードは実際にはまずないでしょう。

ですが、
Aがgood で、BがBad というのは間違っていて
どちらかといえば Aは許容できるけどわずかに Bad
B は good としかいいようがないです。

ここらへんの A のような JS の ちょっとBad な書き方をどの程度許容するのかが、あいまいさがあって
なにせ微妙な Bad なコードが短く書けるからといって大勢の人が使っているので
どうしようもない、という事になっています。

で、こういうBadなJSコードが蔓延した挙げ句に、JSだとバグが発生しやすいからTS使いましょ
という流れになっているのですが、残念な所。
JSでGoodなコード書いていれば、TS使うまでもなくバグが発生しにくくなります。

ほんとうにひどい下手な人たちが書いた
if(foo) のときに、foo に空文字もnullもundefinedも0も入ってくるような
コードをリファクタリングしなければならない経験をしないと
こういう感覚はなかなか得られないです。

あと、他言語ネイティブ(静的型ネイティブ)だとそこもわからないでしょう。
他言語ネイティブな人も多くまざって、書き方を決めたりするから、idomatic のようなのが
多数派になってしまうのは、JSの興味深く難しいところかと思っています。

iwbjpiwbjp

「5.極端に数値が多いときは短くする書き方もある」に関しては
const num = 1e8 にすると短くなるが
数値がいくつなのかわかりにくくなるのでbad!

const num = 100_000_000 のように
数値の区切り文字を使うのがgood!

const num = 100_000_000
console.log(num) // 100000000
TCTC

ありがとうございます!
確かにこちらの方が読みやすいので、記事を修正しました。