JavaScript
初心者
初心者向け

駆け出しエンジニアが受けたフロントエンドのコードレビューをさらす


はじめに

前回の記事 では駆け出しrailsエンジニアとして受けたレビューを紹介しました。

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

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

いってみよう。


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

parentElement等のhtml構造に大きく依存するDOM取得方法は避けること。

html構造に依存している=htmlの変更でjsがすぐバグる

ということなので、マークアップエンジニアやコーダーの人がjsの実装を気にしなければいけなくなってしまう。


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


NG

<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>



OK

<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つけたいとき


NG

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

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



OK

<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として切り出してあればキャッシュされる。



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


NG

$button.addEventListner("click", function(e){

console.log(e.target)
...
});


OK

$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したいとき


NG

$tr.appendChild($td1);

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


OK

const $fragment = document.createDocumentFragment();

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

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


最後に

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