nekoroll.hatenablog.com
前回のコードをAB先生がリファクタしてくれました
なので、自分なりに読み解いて技を盗んじゃおうってやつです
f:id:nekorollykk:20200915230201p:plain

リプライとソースセットで読み解いていこう

AB先生が想定していたコード

僕が想定していたのはこんな感じですね↓
実際にはBoxモジュールが出来て、そこにメソッドが増えていく感じです。このやり方の良いところは、Boxの色の種類が増えたとしても Box自体の性質とBoxを処理しているところで分割統治できるところですね https://t.co/Ib3pBPyN9V
— ABAB↑↓BA (@ababupdownba) 2020年9月14日

関数

`invertBox`関数

invertBox : Box -> Box
invertBox box =
    case box of
        DarkBox p ->
            LightBox p

        LightBox p ->
            PinkBox p

        PinkBox p ->
            DarkBox p

Boxを渡すと次の色のBoxを返すinvertBox関数
僕のケースだとDark <-> Lightだったけど多色対応している
Point(座標)はそのまま引き継いでBoxの種類だけ変化する
→これは何となくイメージできていた(と思う)

`getColor`関数

getColor : Box -> String
getColor box =
    case box of
        LightBox _ ->
            "red"

        DarkBox _ ->
            "white"

        PinkBox _ ->
            "pink"

Boxの種類に応じて色を取得するgetColor関数
これが完全に思いつかなかった
僕のケースだとBoxの種類に応じてdivを返すようになっていてコードの重複が気になっていた
この関数だとBoxを渡すと色が返ってくるので、シンプルで凄い
Boxのdivを生成する処理でもbackground-colorにこの結果を渡すだけでOKなので
コードの重複が無くなる🆒

`getPoint`関数

getPoint : Box -> Point
getPoint box =
    case box of
        LightBox p ->
            p

        DarkBox p ->
            p

        PinkBox p ->
            p

Boxから座標を取得するgetPoint関数
レコードじゃなくてカスタム型になっている強みが出ていると思う
元々想定していなかった多色対応があっさりできていて凄い
レコード型だったらisLightOnの形を変える必要があるけれど、カスタム型なら不要
新たな色の型を追加するだけで対応できる🆒

どの関数も「Boxを渡して処理する」という形で実装されている
List.map 関数 model.boxListとして処理する事が多いと思うけど
どれも関数に渡しやすい関数になっている
この辺が関数型プログラミングのキモなのかな🐊

各関数の使い方

`update`

update : Msg -> Model -> Model
update msg model =
    case msg of
        InvertLight box ->
            let
                invertLight currentBox =
                    if getPoint currentBox == getPoint box then
                        invertBox box

                    else
                        currentBox
            in
            { model | boxList = List.map invertLight model.boxList }

無理やりパターンマッチで処理した僕のコードと違ってシンプル
invertLight関数は引数にcurrentBoxを取り

getPoint currentBox でリスト内の1Boxの座標を取得
getPoint boxでクリックしたBoxの座標を取得
一致していたらinvertBox関数に渡し色を変化させる
不一致なら処理せずそのまま帰す

僕の実装だとif b.x == x && b.y == y thenとしていたけれど
この書き方のほうが「Boxの座標を取得して比較している」という意図がわかりやすい
また、Pointのみに依存しているので仮にx, y, zになっても
getPointの修正のみでOKなので、他処理に影響が出ない。すごい

showBox

showBox : Box -> Html Msg
showBox box =
    div
        [ class "box"
        , style "background-color" <| getColor box
        , onClick (InvertLight box)
        ]
        []

僕のコードだとBoxでパターンマッチして色違いのdivを返す重複の多いコードだったけど
この書き方だと本来分岐させたいbackground-colorのみに影響が留まっている
僕もbackground-colorだけ分岐したいとは思っていたけど、出来なかったので悔しい

ついでに学ぶパイプ演算子

f:id:nekorollykk:20200916010928p:plain
Basics - core 1.0.5

今回のケースだと以下の3つは等価

style "background-color" <| getColor box
style "background-color" (box |> getColor)
style "background-color" (box getColor)

ずっと「何だこれは…？f xでいいのでは…？🤔」と思っていたけれど
今回のケースでようやく読めるようになって、利点も分かった
せっかくなので理解を深めるために個人的な解説をしていく

`f x`

style "background-color" (box getColor)
何の変哲もない関数適用である
散々書いているだけあってわかりやすい

ただ、関数に関数を渡して…ってやっていくとちょっと直感的じゃ無くなる
例) String.toInt (String.trim str)
日本語も英語も左から右に読むようになっているので、一瞬だけ混乱する
🐊「Intに変換する処理…の前にstrをtrimして空白を削るのか」
🐊「strをtrimして空白を削って、Intに変換する処理か」
左から右に読むと順番が前後してしまう
このぐらい短いと行けるけど、もっと長かったり複雑だと苦しいかも知れない
(そもそもそれは関数に切り出すべきというツッコミは置いといて…)

`|>`

https://package.elm-lang.org/packages/elm/core/latest/Basics#(|%3E)
style "background-color" (box |> getColor)
今回のケースだと全くマッチしていなくて無駄である
読んでそのまま「パイプでboxを渡してgetColor関数を適用する」
なんかで見たことあるなーと思ったらUnixのパイプだった(ps aux | grep phpとかね)

生きるのは先程の例String.toInt (String.trim str)のように適用を組み合わせるケース
例を|>に置き換えると以下のコードになる
str |> String.trim |> String.toInt
🐊「strをtrimしてIntに変換する処理か」
何ということでしょう、匠の手により前後が無くなり🐊がスムーズに読めています
直感的に読めて、書ける。なんて素晴らしい演算子…
基本的にこれ使っていこう、って思ったらこんな注意書きがあった

https://guide.elm-lang.jp/appendix/function_types.html#パイプライン

パイプラインは多くの人が好むように、"左から右へ"読むことを可能にするため、コードをすっきりさせることができます。
しかしパイプラインは過度に使われる可能性があります。
3、4つのパイプラインとなった場合トップレベルにヘルパー関数を書いた方がコードがより簡潔になる場合が多くあります。
トップレベルに定義することで変換自体に関数名が付き、その引数にも名前が付けられ、型注釈も書くことになります。
つまりソースコード自体がその意味するところを雄弁に語るようになり、チームメイトや将来の自分自身は感謝することでしょう！
更にロジックをテストすることもより簡単になります。

便利だけど使いすぎは厳禁。やはり基本は関数化
細かく関数化した処理をviewとかで適用するときに使うとか
関数内で他関数を適用するときに使うぐらいが適切かな

`<|`

style "background-color" <| getColor box
https://package.elm-lang.org/packages/elm/core/latest/Basics#(%3C|)

今回のコードで一番適切な演算子だと思う
style "background-color" "" <- ここにgetColor box`を入れる
という感じですごい直感的に読める

基本的には|>の方が使われると思うけど、今回のケースのような
カッコがないほうが直感的に読めるケースで使うとよさそう

style "background-color" <| getColor box
style "background-color" (box getColor)

個人的にはカッコがないほうが読みやすいと思ったけど、好みの領域な気もする
カッコでの書き方は慣れてきたのであえて<|使うようにしてみたいお気持ち💭

`<<` `>>`

🐊💤
残念ながらワニの脳みそでは理解できなかったようです
合成はまだしないしまた合う日まで物置にしまっておきます

知っておくべきありがたいご指摘

あと使ってなかったから消したのですが、

import List.Extra as Ex

の部分。これだとDict.Extra などを並行して使おうとした場合に、Exで被ってしまいます

で、Extraシリーズの多分推奨している使われ方ですが

import List.Extra as List

と書けちゃいます。つまりListモジュールをExtra出来ます
— ABAB↑↓BA (@ababupdownba) 2020年9月14日

import List.Extra as Exを使おうとして結局自分で再実装したまま残してました(反省)

import List.Extra as Listが面白くて
core.Listと重複するものがないから正に拡張になる
当然List.groupsOfが使えるしcoreのList.allもList.mapも使える

たまたまAB先生が教えてくれたからList.Extra as Listを知ったけど
こういうベストプラクティスってどこにあるんだろう？
List.ExtraのドキュメントにもGithubにも無かった(と思う)
こういう細かいベストプラクティスがまとまった何かがあると凄い嬉しい

`Point`と`Box`は分けるべきという話

どうやってDarkとLightを反転させるか…ばかり考えていたせいで、色の処理を分割する発想が全くありませんでした…
同じ処理でもここまでキレイになるんですね…

カスタム型を使わずにレコードで書いてしまい、Elmの良さを活かしきれていないので意識して実装してみます。
本当にありがとうございます
— 🐊すがわに🐊 (@nek0roll) 2020年9月15日

僕が始めPointで分けた方が良いのかな？って違和感は、色と座標は一緒に考える必要はなく座標の一致などが単に比較だけで出来るので分離した方が良かったっていう結論ですね

色は別にboolで扱っても良いんですが、今後拡張するならカスタム型にしてもありかな？ってだけなので蛇足と言えば蛇足です
— ABAB↑↓BA (@ababupdownba) 2020年9月15日

は「対象のモノの役割」をちゃんと考えて適切なコードに落とし込むが出来ていなかった
AB先生が書いている通り、色と座標は別で処理するべきで一つの塊のレコードとして持つべきではなかった
色と座標を分けることでコードがキレイになるし、意味も明確になる
仮に色が多色増えても座標に影響は出ないし、座標にzが増えても色に影響は出ない
と適切に責務が分離したコードを書くことが出来る
これElmに限らず業務のプログラムでもとても大切なことだと思う

レコード同士の比較が出来る

なるほど！色と座標はセットで1レコードとしていましたが、確かに責務？役割？としては別なので、分離するのが正しいですね！

現状点灯/消灯のみなのでBoolでもOKですが、今後色を変える可能性がゼロではないと考えると、拡張性を意識してカスタム型とするのがベストですね…勉強になります。
— 🐊すがわに🐊 (@nek0roll) 2020年9月15日

はいそうするとレコードの比較で座標比較ができます(元コードがx,y取り出して比較していたので) もしくはレコードの比較知らなかったら認知しておくと良さそうです！
— ABAB↑↓BA (@ababupdownba) 2020年9月15日

(知らなかった)
box.x === xってやっていたけどBox PointとすることでPoint同士の比較が出来る
つまりこれもレコードの形に依存せず比較が出来る
box.xだとzが増えたときに比較のロジックも修正が必要になるけれど
Pointの比較にしているとPointにzを足すだけで比較のロジックは影響なし

なんだこれElmすごすぎないか

AB先生のリファクタ最終形

ellie-app.com

ちなみに今回のコード Box自体をカスタムタイプでやる方法はあまり良くありません・・・が、色の部分をカスタムタイプでやるのは悪くないと思います。(なので、こうしたらどうなるかなー？みたいな問いかけで言ってました)

僕だったら最終的にこうリファクタします↓https://t.co/65Okvpvg44
— ABAB↑↓BA (@ababupdownba) 2020年9月14日

`Model`

type Color
    = Dark
    | Light

type alias Box =
    { point : Point
    , color : Color
    }

色付きBox+座標だったものが更に分離されて
Box+座標+色になった
Boxはただのマス目の1つなので、確かに色とBoxがセットは変だった？かもしれない

関数

`makeBox`

makeBox : Int -> List Box
makeBox y =
    List.map
        (\x ->
            let
                point =
                    { x = x, y = y }
            in
            { point = point, color = Dark }
        )
        (List.range 0 6)

let inを使ってまず座標を変数に格納しておいて
その後座標とデフォルトの色Darkを指定してBoxを生成

Boxは型エイリアスでレコードコンストラクタが生まれるから
生成するときにBox point Darkと書くことも出来ると思うんだけど
明示的に{ point = point, color = Dark }と書いたほうがいいのかな？

`turnColor`

turnColor : Color -> Color
turnColor color =
    case color of
        Light ->
            Dark

        Dark ->
            Light

Colorを受け取って色を反転させる
Box要素が消滅して色だけの責務になった。キレイ

`toBackgroundColor`

toBackgroundColor : Color -> String
toBackgroundColor color =
    case color of
        Dark ->
            "white"

        Light ->
            "red"

viewで使用する色の定義を返す
こちらもBox要素が消えて色だけの責務になった。わかりやすい

`update`

type Msg
    = InvertLight Box

update : Msg -> Model -> Model
update msg model =
    case msg of
        InvertLight { point, color } ->
            let
                invertLight currentBox =
                    if currentBox.point == point then
                        { currentBox | color = turnColor color }

                    else
                        currentBox
            in
            { model | boxList = List.map invertLight model.boxList }

InvertLightはBoxを引数に取る、引数の時点で展開可能なので展開(前教わった)
BoxがPointをレコードとして持っているのでgetPoint関数が消滅して
直接座標の比較が可能になった。凄い直感的
クリックしたBoxと座標が一致してるものはturnColorで色を反転させる
Boxを返すのではなく色を変える。というコードになった

AB先生の大型リファクタで一番感動したのがupdate関数
キレイに責務を分けているのでupdate関数内の色更新処理も凄いキレイ
僕のレコードでデータを持って更新する処理と似ているんだけど、完全に似て非なるものでこっちの方が可読性も保守性も圧倒的に高い
クドいようだけど本当に感動した

`showBox`

showBox : Box -> Html Msg
showBox box =
    div
        [ class "box"
        , style "background-color" <| toBackgroundColor box.color
        , onClick (InvertLight box)
        ]
        []

色の指定がBoxではなくColorのみになった
BoxとColorの責務を分けているおかげで、ここのコードもわかりやすくなっている

まとめ

それぞれの役割を理解してちゃんと責務を分けることが大切
Jsonのデコードではある程度考えたつもりだったけど、処理するコードで全く考えられていなかった
特にカスタム型や型エリアスの扱いがまだまだ未熟で、練習が必要だなと感じた
カスタム型で分けつつ、パターンマッチを使って安全にわかりやすく処理すべき
パイプ演算子わかりやすくてオシャレなので使いたい
自分が実装した下手なコードを他者であるAB先生が直せるの凄くない？
細かい設計なんて一切話していないし、こうしたいもほとんど話していない
AB先生の力ももちろんあるんだけどElmの堅牢さの真髄を見た気がする
実際自分でリファクタするときもコンパイラに従うだけで、リファクタを進めることが出来たので、噂通りホントにコンパイラが強い言語だなと思った

めっちゃ長文になってしまった
AB先生のリファクタ凄いのとElm凄いという気持ちでいっぱい
この文章書いているうちに以前作ったTODOリストリファクタしたくてたまらなくなってきた🐊
でも今はSTACKERゲーム完成を優先で、もっと力つけてからリファクタチャンレジしよう

繰り返しになりますがAB先生本当にありがとうございますm( )m

わにろぐ

STACKERゲームを作ろうその2.9999

AB先生が想定していたコード

関数

`invertBox`関数

`getColor`関数

`getPoint`関数

各関数の使い方

`update`

showBox

ついでに学ぶパイプ演算子

`f x`

`|>`

`<|`

`<<` `>>`

知っておくべきありがたいご指摘

`Point`と`Box`は分けるべきという話

レコード同士の比較が出来る

AB先生のリファクタ最終形

`Model`

関数

`makeBox`

`turnColor`

`toBackgroundColor`

`update`

`showBox`

まとめ

AB先生が想定していたコード

関数

invertBox関数

getColor関数

getPoint関数

各関数の使い方

update

showBox

ついでに学ぶパイプ演算子

f x

|>

<|

<< >>

知っておくべきありがたいご指摘

PointとBoxは分けるべきという話

レコード同士の比較が出来る

AB先生のリファクタ最終形

Model

関数

makeBox

turnColor

toBackgroundColor

update

showBox

まとめ

`invertBox`関数

`getColor`関数

`getPoint`関数

`update`

`f x`

`|>`

`<|`

`<<` `>>`

`Point`と`Box`は分けるべきという話

`Model`

`makeBox`

`turnColor`

`toBackgroundColor`

`update`

`showBox`