JSのお勉強 公開コードレビュー その1

ひなたみ @hinatami

@kyo_ago @edo_m18 #日暮里JS 先生方！とりあえず1つユーザースクリプト書いてみて、GitHubに上げてみた（つもりな）んですけど、こんな感じで大丈夫なのでしょうか？GitHubよくわからなすぎて＞＜ github.com/hinatami/study

kyo_ago @kyo_ago

@hinatami @edo_m18 おーすごい！こっちでも見れてますー ではこれに対して突っ込める範囲で突っ込んでみますー

ひなたみ @hinatami

@kyo_ago @edo_m18 わー！よろしくお願いしますー！こわいーｗ

kyo_ago @kyo_ago

@hinatami @edo_m18 READMEがMDで書かれてるのいいですね。promote-request-kindle.user.jsの先頭のコメントもポイント高いです。全体のコードで言うとわりとクラシカルな印象で、実際にブラウザ上で動かすことを想定しても

kyo_ago @kyo_ago

@hinatami @edo_m18 もう少しAPI使っても大丈夫です（もちろんターゲットブラウザにもよるけど）例えば、document.querySelectorはIE8から使えるので案件でも使える範囲だと思います

kyo_ago @kyo_ago

@hinatami @edo_m18 実際DOMアクセス時に「DOMを取得してforで回す」のはかなり最後の手段で、通常はquerySelectorAllで取得して判別することが多いです（ただ、ユーザスクリプト系の「他人のコンテンツ勝手に扱う系」は止む終えない場合はあるけど）

kyo_ago @kyo_ago

@hinatami @edo_m18 あとはwindow.onloadも基本的に禁じてと思ったほうがいいです。まず、on*でイベントをbindしてる点が一点と、loadイベントを使っている点が問題になります（ユーザスクリプトでは問題視しないこともあるけど、実案件ではやらない）

kyo_ago @kyo_ago

@hinatami @edo_m18 「なぜon*がまずいか」ですが、これは「複数箇所から同時にイベントをbindできない」という問題があって、具体的に言うとwindow.onload = fun1; window.onload = func2;というコードが合った場合、

kyo_ago @kyo_ago

@hinatami @edo_m18 func1が実行されません。で、「loadイベントを使っている」件は、基本的にJSの発火はDOMContentLoadedイベントを使います（が、ユーザスクリプトだと、初期読み込みを優先させるためにあえてloadイベント使うこともあります。

kyo_ago @kyo_ago

@hinatami @edo_m18 あと、ユーザスクリプトだとon*使っても問題なかったりもしますが、一応「実案件では」という前提で上げてます）

ひなたみ @hinatami

@kyo_ago @edo_m18 えっと、（実案件では）document.addEventListener("DOMContentLoaded", function・・・を使うと良い、という理解で大丈夫でしょうか？（ちゃんとついていけなくてスミマセン＞＜）

kyo_ago @kyo_ago

@hinatami @edo_m18 あ、そのままだとクロスブラウザ問題あるので、基本的には「window.onloadを使わない」という理解をしてもらえれば大丈夫です（「実案件では$(func)使う。ユーザスクリプト系ではDOMContentLoaded使う」と思って下さい）

ひなたみ @hinatami

@kyo_ago @edo_m18 ごめんなさい、$(func) がわからなかったです…（ググりづらい）

kyo_ago @kyo_ago

@hinatami @edo_m18 あ、たしかにググれないですね。これはjQuery(func)の省略記法で、 // jQuery：jQueryを実行する時のready()の記述方法 | raining raining.bear-life.com/jquery/jquery%…

kyo_ago @kyo_ago

@hinatami @edo_m18 機能的にはonloadと同じなんですが、onloadが画像の読み込みを待ってしまうのに対して、readyは待たないので早いタイミングで実行されます（画像が重いページでは特に実行速度に差が出る）

ひなたみ @hinatami

@kyo_ago @edo_m18 あっ、なるほど jQuery！ありがとうございます！ちなみに、jQuery 使わない実案件というケースがあったら、何を使ったらいいんでしょう？（今のところそんなケースは特にないですが）

kyo_ago @kyo_ago

@hinatami @edo_m18 一応<script>ここにコードを書く</script></body>ことでDOMContentLoadedと同じ効果があります（が、jQuery使えない案件、かなりレベルが高いのであんまり考えなくていいかも。。。）

ひなたみ @hinatami

@kyo_ago @edo_m18 基本的には jQuery 使うってことなんですね、なるほど。（そこから？っていう話でした…）ありがとうございます！

kyo_ago @kyo_ago

@hinatami @edo_m18 あと、これはわりと個人の趣味の問題もありますが、insertBeforeよりinsertAdjacentHTMLとかinsertAdjacentElementの方が使い勝手よくて好きだったりします（IE4位から使えるし）

kyo_ago @kyo_ago

@hinatami @edo_m18 あと、エラー処理がされてない問題もあって、@ matchはwww.amazon. co.jp*ですが、amazonのトップで実行すると# fiona-publisher-signup-linkがな

kyo_ago @kyo_ago

@hinatami @edo_m18 くてエラーになります（forでkindle版が存在する場合に実行しないようにしてると思いますが、そもそも# fiona-publisher-signup-linkがない場合の想定が必要）

ひなたみ @hinatami

@kyo_ago @edo_m18 ( ﾟдﾟ)ﾊｯ! なるほど。そうですね！（これは理解できます！）

kyo_ago @kyo_ago

@hinatami @edo_m18 と、いう感じで私からは以上なので次、edoさんお願いしますー

edom18@XR / MESON CTO @edo_m18

@hinatami @kyo_ago なんかほとんど指摘されちゃってるので、あまりないですがｗ　あ、ちなみにjQuery以外となると結構むずかしいかも。案件による、という感じで選ぶことになるはずだから。

edom18@XR / MESON CTO @edo_m18

@hinatami @kyo_ago コードで気になったところは「for (var i = 0; i < kindleChildren.length; i++) {」のところかな。