Movatterモバイル変換


[0]ホーム

URL:


Skip to content
Search Gists
Sign in Sign up

Instantly share code, notes, and snippets.

@yamachu
CreatedFebruary 3, 2021 09:25
    • Star(18)You must be signed in to star a gist
    • Fork(0)You must be signed in to fork a gist
    Save yamachu/01048e865836b7f729d2b85d203c366f to your computer and use it in GitHub Desktop.
    JavaScriptのCollection操作をする時はお願いがある、一呼吸置いてから操作をしてくれ

    私がJavaScriptで書かれたコードを読んで息が詰まるケース…今回はCollection操作についてをご紹介します。また私はレビューする時cloneしてから臭う変数はリネームをかけて「なんかに使ってそうでどこまで使われてるかわからん」みたいな日本語に置換してからレビューをするレベルで同一スコープの変数管理が苦手な属性を持っています(なんの紹介)。

    最近私がSlackやTwitterで色々言ってることです。難しいですよね、Array.prototype.sort

    Arrayをsortして、その結果を元になにかの処理するみたいなこと、めちゃくちゃやってると思います。めちゃくちゃ使うメソッドではあるけれども、MDNの返値の所に書かれている

    ソートされた結果の配列。ソートは対象配列上で in place に行われることに注意して下さい。コピーされた別の配列が準備されることはありません。

    これを意識していますでしょうか。

    どういうこと?と思われるかもしれませんが、以下のコードを見ればどういうことかわかるかと思います。

    constarr:Array<number>=[5,4,3,2,1];constsorted:Array<number>=arr.sort();// sorted => [1,2,3,4,5]// arr => [1,2,3,4,5]

    MDNのドキュメント通りsortedにはarrのsort済みの配列が格納され、結果も望んだものとなっています。しかしarrはconstで宣言したにも関わらず値が書き換わっています。Array.prototype.sort は破壊的なsortなのです。Pythonでいうsortedじゃないsortみたいな振る舞いです。

    関数のコールの深さが浅ければこのsortされた元の配列が次にどこで使われているか、それはsortされていても問題ないかが追うことができますが、ある程度の深さになった場合追い切ることは困難になります。

    sortが目的なのか、sortされた配列を元に何かしらの作業をすることが目的なのかとか、メモリ制限があってとか、色々と事情によって変わりますが、特に制約はなくsortされた配列を元に何かしらの作業をすることが目的であれば安易にArray.prototype.sort を使わないでくれた方が、私は嬉しいです。

    ではどうすればいいか。色々と方法はありますが、

    constarr:Array<number>=[5,4,3,2,1];constsorted:Array<number>=arr.slice().sort();// sorted => [1,2,3,4,5]// arr => [5,4,3,2,1]

    みたいにArray.prototype.sliceを使って浅いコピーを作ってからsortをするなどしてみてください。これで元の配列の要素の並びに関しては守ることができます。

    とは言えこれをいつも覚えて実装するのは難しいでしょう。そういう時のためのlintってことでDisallow mutating objects and arrays (immutable-data)のルールを使ってみると、ルールの設定によってはfilter後のsortも許さんぞみたいなふるまいで使いづらそう…

    なのでTypeScriptで書いている時は

    constarr:ReadonlyArray<number>=[5,4,3,2,1];

    の様に変数をReadonlyArray<T> として置いてしまうと安全に扱うことができます。この状態でsortを行おうとすると

    Property 'sort' does not exist on type 'readonly number[]'.(2339)

    という風に怒ってくれます。もちろんこれは関数の引数の型にも指定できるので、参照しか行わない、という強い気持ちを示したい時は積極的に使っていきたい型です。

    他にもArray.prototype.sortは難しい要素があります。numberの様な減算によって大小が一意に確定する要素の比較では問題にはなりませんが、文字列などの比較や、オブジェクト内のプロパティで比較を行う際に実装を誤ると意図しない挙動になったりします。

    例えば

    constcompareFunction=(a:string,b:string)=>{returna>b ?1 :-1;}

    みたいにbooleanを大小比較の 1, -1 のみにした場合、環境によってはこの結果の順序が一意に定まりません。MDNに書かれているように

    functioncompare(a,b){if(ある順序の基準においてabより小){return-1;}if(その順序の基準においてabより大){return1;}// a は b と等しいはずreturn0;}

    この様に一意に定まるようにすることが望ましいでしょう。どういう基準であるかを明確に書いてあるコードを読むと、じゃあ値が同じ場合はこういう振る舞いをするんだなと私が安心して読むことができます。

    https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#description

    compareFunction(a, b) が 0 を返した場合、a と b は互いに変更せず、他のすべての要素に対してソートします。注意: ECMAScript 標準はこの振る舞いを保証していないため、一部のブラウザー (例えば、遅くとも 2003 年以前のバージョンの Mozilla) はこれを遵守していません。

    みたいな感じで、古い環境ではこれを遵守してくれる、とは限らないのですが(https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility の stable sortingを参照)。

    Array.prototype.map

    一日に100回は書くんじゃないかってぐらいめちゃくちゃ便利なArray.prototype.map。多くの場合は

    constarr:Array<number>=[5,4,3,2,1];constdoubled:Array<number>=arr.map(x=>x*2);// doubled => [10,8,6,4,2]

    みたいな感じで戻り値を使うのですが、稀に使わないコードを見ます。そういう時は大体日本語で変数名を置換しなければいけないブツが転がっています。

    そう例えば

    constcounter:Map<number,number>=newMap()constarr:Array<number>=[1,1,2,3,5];arr.map(v=>{constbefore=counter.get(v)??0;counter.set(v,before+1);});/*counter =>  0: {1 => 2}  1: {2 => 1}  2: {3 => 1}  3: {5 => 1}*/

    みたいなmapから外部の変数の状態を書き換えているコードです。Array.prototype.map のページの一行目に書かれているように

    map() メソッドは、与えられた関数を配列のすべての要素に対して呼び出し、その結果からなる新しい配列を生成します。

    主目的としては、map内の関数の戻り値から新しい配列を作ることです。私はmapが書かれていたら値にどういう変換関数をかませるのかなというのを見て、その戻り値が適切な名前で変数として割り当てられているかというのをレビューで見ています。戻り値がなかった瞬間これは、どこに消えたんだ…………と混乱していまいます。

    外部の変数の操作が目的なのであれば、お願いです、Array.prototype.forEachとかfor...ofを使ってください。Array.prototype.forEachは渡された関数を各要素に実行することが目的の関数のため、読んでいてもいっぱいこれをするんだなーぐらいの能天気さで読むことができます。

    先程挙げたコード例でも要素をMapに突っ込むことが最終目的で、それの為に行っていることは…という風に読めばいいという風に思考を変えることが出来るからです。

    余談ですが、それでも上記コードのスコープが十分に短くないとforEachを使っていようが詰まるときがあります。Map という型で宣言されているため、このスコープ内で他に状態を変える操作が行われていないかの調査をする必要があるからです。これもReadonlyMapとして宣言できればそういう心配もなくなるのですが、それを行うためには

    constarr:Array<number>=[1,1,2,3,5];constcounter:ReadonlyMap<number,number>=arr.reduce((prev,curr)=>{constbefore=prev.get(curr)??0;constnext=newMap(prev);next.set(curr,before+1);returnnext;},newMap<number,number>());

    みたいに流石に大げさすぎんか?みたいなコードにもなってしまうこともあるので、命名やスコープを限っていい感じのコードにしていけると良いと思います。

    終わりに

    最初辺りにも書いたけれども、色んな制約のもとコードを書いているわけで、今挙げたことをすることが別に悪ってわけではない。パフォーマンスのためだったり、動作環境の制約だったり、そういうのが色々あることも重々に承知している。その中で自分が、周りが、読んで意図を汲み取れるコードが書けて、そのコードが価値を出しているのであれば正直なんでもいい。

    けれども、もしも、もしも安全なコードに近づけたい、読み手の負荷を減らしたいという思いが少しでもあるのであればこの記事だったりを思い出してくれると嬉しい。

    そんな偉そうに語ってるお前はどうなんだ?と問われると

    大体Array.prototype.reduceで書いて「ハンマーしか持っていなければすべてが釘のように見える」みたいになっていますテヘペロ

    キーワード

    • 破壊的
    • Immutable
    • 副作用
    • 参照透過性
    @munierujp
    Copy link

    munierujp commentedFeb 4, 2021
    edited
    Loading

    流石に大げさすぎんか?みたいなコードにもなってしまうこともあるので、命名やスコープを限っていい感じのコードにしていけると良いと思います。

    配列などのイテーラブルな値をもとにMapをつくりたい場合、その部分の処理を丸ごと関数に切り出すとミュータブルな変数のスコープが短くていい感じになりますね。

    constcount=(...nums:number[]):ReadonlyMap<number,number>=>{// この counts はミュータブルconstcounts=newMap<number,number>()nums.forEach(num=>{constbefore=counts.get(num)??0// ミュータブルなのでここでは set() できるcounts.set(num,before+1)})returncounts}// この counts はイミュータブルconstcounts=count(1,1,2,3,5)// イミュータブルなのでここでは set() できない//@ts-expect-errorcounts.set(1,-1)

    これは、やむをえずlet宣言を使わなければならないときにも使えるテクニックです

    Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

    [8]ページ先頭

    ©2009-2025 Movatter.jp