はじめに

前回の記事では駆け出しrailsエンジニアとして受けたレビューを紹介しました。
フロントエンドの実装にも首を突っ込む機会があったので、今回はjs(厳密に言うとtypescriptですが)の実装で受けたコードレビューを紹介しようと思います。

晒してばっかりですが、駆け出しのうちは恥をさらしてなんぼではないでしょうか。
いってみよう。

マークアップとの依存を低くすることを意識しよう

parentElement等のhtml構造に大きく依存するDOM取得方法は避けること。
html構造に依存している=htmlの変更でjsがすぐバグる
ということなので、マークアップエンジニアやコーダーの人がjsの実装を気にしなければいけなくなってしまう。

例: ボタンのクリックイベントでそのボタンが属する`<tr>`をとってきたいとき

<table>
  <tr>
    <td><button id="ng-btn" type="button">NG</button></td>
  </tr>
  ...
</table>

<script>
const $button = document.querySelector("#ng-btn");
$button.addEventListner("click", function(){
            <!-- このとり方だと<td>直下がボタンじゃなくなったら動かなくなる -->
            const $tr = $button.parentElement.parentElement;
            ...
        });)
</script>

<table>
  <tr class="hoge-row">
    <td><button id="ok-btn" type="button" data-raw-class='hoge-row'>OK</button></td>
  </tr>
  ...
</table>

<script>
const $button = document.querySelector("#ok-btn");
$button.addEventListner("click", function(){
            const $tr = document.querySelector(`.${$button.dataset.rawClass}`);
            ...
        });)
</script>

だいぶ雑な例示ですが、大雑把な方針としては、

子→親の直接の取得はhtml構造に必ず依存するので避ける
子には親に関する情報をもたせた上で、その情報から改めて親を取得する

jsにstyleの情報を持たないようにしよう

styleの管理はcssにまとめたほうが煩雑にならない。
jsではclassの付け替えをする等にとどめておき、cssでそのclassにたいしてスタイルを振れば良い。

例: 動的にdisplay:noneつけたいとき

<div id="ng">ng</div>

<script>
const $div = document.querySelector("#ng");
$div.style.display="none"
</script>

<style type="text/css">
.is-hidden {display:none;}
</style>

<div id="ok">ok</div>

<script>
const $div = document.querySelector("#ok");
$div.classList.add('is-hidden')
</script>

パフォーマンスを意識できるようになろう

addEventListenerを使うときは無名関数は使わない

addEventListenerを使うときは無名関数は使わないようにする。
新しくfunctionを切ってそれを呼ぶようにする。

メリットは大きく以下

そのlistenerを使い回せる
→ 他のeventにも使える
eventを削除できるようになる
→ 不要なeventが残ってしまうのは健全ではない。無名関数だとremoveするのに苦労する。
メモリ的にもやさしい
→ 無名関数だと呼ばれるたびにメモリを新たに消費するが、functionとして切り出してあればキャッシュされる。

例:あるボタンへのクリックイベント

$button.addEventListner("click", function(e){
            console.log(e.target)
            ...
        });

$button.addEventListner("click", _handleBtnClick);

function _handleBtnClick(e){
  console.log(e.target)
  ...
}

appendChildは毎回リフロー・リペイントされるから工夫して使う

そもそもリフロー・リペイントとは。

リフロー・・・各HTML要素を解釈して、それぞれが占めるスペースを算出する
リペイント・・・算出したスペースのなかに、内容を描画していく

「Reflowを制するものはDOMを制す」

そしてリフロー・リペイントは以下をトリガーとしておきる。

DOM ノードの追加、削除、更新

display: none （リフローとリペイント）、あるいは visibility: hidden （位置の変更は起きないので、リペイントのみ）による DOM ノードの見た目の変更

ページ中の DOM ノードの位置の移動やアニメーション

スタイル属性のちょっとした変更のためのスタイルシート追加

windowサイズの変更やフォントサイズの変更、そしてスクロールなどの、ユーザーの操作

「ブラウザ動作の理解－リフローとリペイント及びその最適化」

appendChildはもろにDOMノードの追加にあたるので、連続で呼ぶのはよろしくない。
こういうときは、createDocumentFragmentを使おう。
描画されているDOMにappendされるまではリフロー・リペイントされない。

例:trタグにtdタグを複数appendしたいとき

$tr.appendChild($td1);
$tr.appendChild($td2);
$tr.appendChild($td3);

const $fragment = document.createDocumentFragment();
$fragment.appendChild($td1);
$fragment.appendChild($td2);
$fragment.appendChild($td3);
$tr.appendChild($fragment);

NGの例では3回もDOMノードの更新が走るが、OKの例では1度で済む

最後に

こういうポイントも意識するべきなどありましたら、ぜひご指摘のほどよろしくお願いします。

フロントエンドの実装デビュー直後に受けるであろうコードレビュー