#前置き
私が所属している「もりけん塾」で受けたコードレビューについてまとめていきます。
「もりけん塾」では、先生がマークアップエンジニアからフロントエンドエンジニアになるための課題を作成してくださっており、塾生はその課題を通してJavaScriptの基礎を学んでいきます。
本当に1段1段階段を登っていくように作られており、課題を終える頃にはある程度自走しながらコードが書けるようになります。
私もこの課題に挑戦し、先日課題を終えることができました。だいたい2~3ヶ月くらいかかったと思います。JavaScriptが全然わからない状態から、ここまで書けるようになるとは思っていなかったです。
JS課題はこちら
成果物 👈 README.mdに置いてます。
#先を見越したコードを書く編
##仕様が増えたときを想定する
バリデーションを実装する課題でのレビューです。
名前のバリデーションでは、変数名を nameLength としており、
例えば名前の正規表現を実装するとなれば nameLength という変数名では追加したい仕様に沿った変数名ではないです。
また、メールアドレスのバリデーションでは正規表現を変数だけで持つようにしていたのですが、これも他の仕様が追加されることを想定して、オブジェクトで管理することを教えていただきました。
先を見据えて変数名やオブジェクトで管理するといった方法を学ぶことが出来た、とても勉強になった課題でした。
try-catch-finally
try-catch-finallyを使ってローディングを実装しました。その際のコードの一部がこちらです。
注目していただきたいのはfinallyの記述です。
async function outputFn() {
try {
const result = await promiseObj();
createElements(result);
}catch(e) {
console.error(e);
}finally {
console.log('処理が終わりました。');
}
};
outputFn();
ここでは、outputFn の処理が終わったということで、コンソールに「処理が終わりました。」と出力されるようにしていたのですが、先生からのレビューは
確かに。resolveされたときにはこれでも良いかもしれませんが、rejectされると処理はcatch節に移るので、その状態では**「処理は終わっていない。または失敗している」**ので、この書き方は間違えているということになります。
修正後はこちら。
async function outputFn() {
try {
const result = await promiseObj();
createElements(result);
}catch(e) {
console.error(e);
}finally {
loading.remove(); // この部分を修正
}
};
outputFn();
今回の課題の場合、処理が終わるまではローディング画像を出していたので、処理が終わったタイミングで loading.remove()とするように記述を変更しました。
どこでも使えるようなロジックは抽象的に書く
これはユーザが持っているIDでソートをする関数です。ここで先生から。
この後の課題は年齢でもソートを行うといったものでした。もし上記のコードのまま年齢のソートをしようと思うと同じようなコードを書く必要がありました。
// idでソート
function sortIdDesc(a, b) {
if (a.id > b.id) return -1
if (a.id < b.id) return 1
return 0
}
// ageでソート
function sortIdDesc(a, b) {
if (a.age > b.age) return -1
if (a.age < b.age) return 1
return 0
}
違うのは .id か .age のみです。これはスマートではありません。修正後はこちら。
// keyに渡す情報でソートする
function sortDesc(key) {
return (a, b) => {
if (a[key] > b[key]) return -1
if (a[key] < b[key]) return 1
return 0
}
}
このように汎用性を持たせてコードを書くことで、余計なコードを書く必要がなくなります。
#命名編
処理が分かる関数名をつける
async function fetchData() {
try {
const response = await fetch('https://jsondata.okiba.me/v1/json/9omPz210202144336');
const json = await response.json();
loading.remove();
createElements(json.data);
} catch(e) {
console.error(e);
} finally {
console.log('outputFunc run');
}
}
setTimeout(outputFunc, 3000);
関数 fetchDataですが、関数名の通りに処理を考えると「fetchしてdataを返すだけの関数」になるはず。
しかし、この関数内では createElementsも実行しており、 fetchDataをしているだけの関数ではなくなってしまっています。この場合、関数名を変更するか、 createElementsは別の処理として切り出す必要がありました。
今回の場合は関数名を outputFuncに変更しましたが、これでは**「outputって何処に?何を?ていうのが分からないのとFuncは隣のfunctionで自明なので全体的に抽象的すぎる気がしています。」**と再度レビューをいただくことになりました。
今考えるとtry節で return json.dataとして createElementsは別の場所に切り出す方法が自然かなと思いました。
Boolean値を扱う時の関数名・変数名
JSONを作成し、最初に表示する画像を決める際に値に trueを持つプロパティを作成していました。
（文字列のtrueになっているのは後々修正しました。）
このプロパティ名が firstViewとなっており、先生から
とレビューを受けました。
今回のように trueを持つプロパティ名や、値があるかないかを判断する関数名には is**や has**を使うようにしています。
例えば以下のようなコード
// is**の例
const content = document.getElementById('content')
content.classList.add('active')
const isActive = (target) => {
return target.classList.contains('active')
}
if(isActive(content)) {
console.log(`content is Active!!`)
} else {
console.log(`content is not Active...`)
}
// has**の例
const comments = ['美味しかった', '早かった', '安い']
const hasComment = (comments) => {
return comments.length
}
if(hasComment(comments)) {
console.log(`コメントあったで`)
} else {
console.log(`コメントなかったわ`)
}
みたいな感じで使っています。
イベントを付与する関数名の付け方
英語力の問題かもしれませんが、
僕はスライドショーの戻る・次へボタン（◀︎ ▶︎ みたいな矢印）に addEventListener('click')を付与したかったのですが、レビュー前は、clickArrowという関数名をつけていました。
先生からは
とレビューをいただきました。
結局自分では答えを出せずにいたのですが、先生が答えを教えてくださいました。
少し関数名は長いですが、 attachClickEventForArrow 確かにこちらは関数名から何をしているのかが伝わる関数名になっていると思います。
リーダブルなコードを書くためには編
##関数の肥大化を避ける
約3ヶ月前、恐ろしいくらい長いコードを書いていました。
これが1つの関数の処理だと思うと気がおかしくなりそうです。こんな冗長なコードをレビューしろと言われた先生の気持ちたるや...。
async function createElement () {
const articleList = await fetchArticleData()
const tab_list = new Set()
for(const article of articleList) {
// タブの生成
const tab = document.createElement('li')
tab.textContent = article.category
tab.classList.add('tab__item')
tabs.appendChild(tab)
tab_list.add(tab)
// どのタブを初期表示にするか
if(article.firstView === 'true') {
tab.classList.add('active')
}
if(article.category === 'ニュース') {
tab.dataset.id = 'js-news'
} else if(article.category === '経済') {
tab.dataset.id = 'js-economy'
} else if(article.category === 'エンタメ') {
tab.dataset.id = 'js-entertainment'
} else if(article.category === 'スポーツ') {
tab.dataset.id = 'js-sports'
} else if(article.category === '国内') {
tab.dataset.id = 'js-japan'
}
// firstViewがtrueのコンテンツを初期表示にする(ニュースカテゴリーがtrueを保持している)
if(article.category === 'ニュース' && article.firstView === 'true') {
newsContents.classList.add('active')
} else if(article.category === '経済' && article.firstView === 'true') {
economyContents.classList.add('active')
} else if(article.category === 'エンタメ' && article.firstView === 'true') {
entertainmentContents.classList.add('active')
} else if(article.category === 'スポーツ' && article.firstView === 'true') {
sportsContents.classList.add('active')
} else if(article.category === '国内' && article.firstView === 'true') {
japanContents.classList.add('active')
}
// タブの生成ここまで
// タイトルの生成
for(const info of article.articleInfo) {
const title = document.createElement('li')
const title_link = document.createElement('a')
const comment = document.createElement('span')
const comment_icon = document.createElement('img')
title.classList.add('title')
comment.classList.add('comment')
title_link.innerHTML = info.title
comment_icon.src = 'img/comment.png'
comment_icon.classList.add('comment_icon')
title_link.appendChild(comment)
title.appendChild(title_link)
// 投稿日と今日との日差を取得
const postDay = new Date(info.postDay)
const ms = today.getTime() - postDay.getTime()
const days = Math.floor(ms / (1000*60*60*24))
// 14日以内の投稿であればnew_iconを追加する
if(days <= 14) {
const new_icon = document.createElement('img')
new_icon.src = 'img/new_icon.png'
new_icon.classList.add('new_icon')
if(article.category === 'ニュース') {
title.appendChild(new_icon)
} else if(article.category === '経済') {
title.appendChild(new_icon)
} else if(article.category === 'エンタメ') {
title.appendChild(new_icon)
} else if(article.category === 'スポーツ') {
title.appendChild(new_icon)
} else if(article.category === '国内') {
title.appendChild(new_icon)
}
}
// コメントがあればアイコンを追加
if(info.comment > 0) {
comment.textContent = info.comment
comment.appendChild(comment_icon)
}
if(article.category === 'ニュース') {
newsTitleWrap.appendChild(title)
newsContentsInner.appendChild(newsTitleWrap)
newsContents.appendChild(newsContentsInner)
} else if(article.category === '経済') {
economyTitleWrap.appendChild(title)
economyContentsInner.appendChild(economyTitleWrap)
economyContents.appendChild(economyContentsInner)
} else if(article.category === 'エンタメ') {
entertainmentTitleWrap.appendChild(title)
entertainmentContentsInner.appendChild(entertainmentTitleWrap)
entertainmentContents.appendChild(entertainmentContentsInner)
} else if(article.category === 'スポーツ') {
sportsTitleWrap.appendChild(title)
sportsContentsInner.appendChild(sportsTitleWrap)
sportsContents.appendChild(sportsContentsInner)
} else if(article.category === '国内') {
japanTitleWrap.appendChild(title)
japanContentsInner.appendChild(japanTitleWrap)
japanContents.appendChild(japanContentsInner)
}
}
// タイトルの生成ここまで
// 画像の生成
const img = document.createElement('img')
img.src = article.img
img.classList.add('img')
if(article.category === 'ニュース') {
newsContentsInner.appendChild(img)
newsContents.appendChild(newsContentsInner)
} else if(article.category === '経済') {
economyContentsInner.appendChild(img)
economyContents.appendChild(economyContentsInner)
} else if(article.category === 'エンタメ') {
entertainmentContentsInner.appendChild(img)
entertainmentContents.appendChild(entertainmentContentsInner)
} else if(article.category === 'スポーツ') {
sportsContentsInner.appendChild(img)
sportsContents.appendChild(sportsContentsInner)
} else if(article.category === '国内') {
japanContentsInner.appendChild(img)
japanContents.appendChild(japanContentsInner)
}
// 画像の生成ここまで
}
tabClickAction(tab_list)
}
だいたい150行くらいありました。クソくらえですね。
とレビューをいただきました。
というわけで関数内部で行なっていた処理を関数化し切り出したコードがこちらです。
async function createElements () {
let articles
try {
articles = await fetchArticle()
} catch(e) {
console.error(e);
} finally {
loading.remove();
}
createTabs(articles)
createTitles(articles)
createImages(articles)
checkContentsIsInit(articles)
}
なんと14行になりました。同じ関数とは思えないですね。
ここで関数の切り出しをめちゃくちゃ練習したおかげで、引数の扱い方が少しわかった気がします。
切り分けたことで関数が肥大化せず、引数に何を渡しているのか、関数名からその関数が何をしている関数なのかということも分かるようになりました。
また、テスト？（まだやったことないのでわからない）をする際にも処理を切り出すことは大切なんだとか。
むやみに関数化しない
処理を切り分けることや、関数化することに楽しさを覚えてしまったころ、先生からレビューが。
tokenがlocalStorageに存在するか を確認する処理を関数化して切り分けたのですが、見事に修正されました。
理由として
- 繰り返し使うものではないと判断した
-
localStorage.getItem('token')で処理を説明できている - 切り分けると冗長
と解説していただきました。
なんでも関数化したり切り分けたりすると、それはそれでリーダブルでなくなってしまうという。
マジックナンバーに気を付けろ
こちらは同じ塾生のもなかさんにレビューしていただきました。もりけん塾では塾生も積極的にコードレビューをすることが特徴です。
コード書いているときには気にならなかったのですが、確かに後で見返すとなんの1かわかりません。
const nextNum = 1
changeImage(nextNum)
changePaginationIncrement(nextNum)
とすることで、1に意味を持たせることができました。
関連する値をオブジェクトとしてまとめる
...この currentNumは一体どこで、どのように使われるのでしょうか。
答えはスライドが今何番目が表示されているかを管理するための値でした。これだと何のために存在するのかをコードを見ただけでは予測できません。
そのため、関連する値はまとめてオブジェクトにして管理してしまうという手があります。
修正後はこちら
const slideState = {
currentNum: 0,
images: []
}
これでスライドに関する値であることが伝わるコードになったかと思います。
さらに僕はここで「分割代入」を知っていたので、使ってやろうと意気込んで使って見ました。
const slideState = {
currentNum: 0,
images: []
}
let { currentNum } = slideState
const { images } = slideState
すると先生から
僕的には何度も記述する値だったので、毎回 slideState.currentNumと記述するのが億劫になっていたので分割代入を使っていたのですが、ここで分割代入をしてしまうと結局 currentNumはどこの・何の currentNumなのかがわからなくなってしまいますね。
if-elseをswitchにしてリータブルに。そして保守性も大切に。
こちらは、コメントに書いてある通り「ユーザが持っているkey情報を元に、表示したいカラム名に変換して返却する」関数です。
if-elseを使うと、「なんの条件の時にどういった処理が行われるか」「あ、いろんな条件でreturnする値を変えているな」というのがわかりやすくなるのがメリットかなと思っています。
このコードに対して先生から
というコメントとともに
function changeColumnName(column) {
switch (column) {
case "id":
return "ID";
case "name":
return "名前";
case "age":
return "年齢";
case "sex":
return "性別";
default:
console.error(`not provided ${column}`);
}
}
このコードが返ってきました。switch文、初めて使うところを見ました。今までは if-elseがあったので、別に覚えなくてもいいわ...くらいに思っていたのですが、
**「読みやすさ」**が全然違う。邪魔な記述がないコードを見ると感動すら覚えます。
そして、先生からのレビューにあった**「どこにも当てはまらない時の処理」**が保守性に関わってくる部分です。（恐らく。）
今は自分が作成したJSONを使ってフロントを作っているので意図しない値が渡ってくることはありませんが、先生は**「ここで要素が増えたら」「もしこんな値が渡ってきたら」**など、今後のことも考えながらレビューをしてくれます。
こんなツイートもされてました。
[HTML tips]
— フロントエンドエンジニア (@terrace_tech) May 3, 2021
contenteditable属性をtrueにするとその要素は編集可能になる
- 文字の増減による表示崩れ確認に便利
- 非推奨
動画は開発ツールでbodyにcontenteditable="true"を与え色々いじっている様子 pic.twitter.com/AtUR8UgHSe
switch文をオブジェクトを使ってさらにリーダブルに
function changeColumnName(column) {
switch (column) {
case "id":
return "ID";
case "name":
return "名前";
case "age":
return "年齢";
case "sex":
return "性別";
default:
console.error(`not provided ${column}`);
}
}
function changeColumnName(key) {
const mapColumns = {
"id": "ID",
"name": "名前",
"age": "年齢",
"sex": "性別",
}
return mapColumns[key] ?? console.error(`not provided ${key}`);
}
なんともいえないすっきり感。
[JavaScript]
— フロントエンドエンジニア (@terrace_tech) May 3, 2021
if elseで一つ一つ評価していく方法もありますが
条件が多い場合見栄えがよくないです
これを...
switch文やオブジェクトを使うと少し見やすくなります pic.twitter.com/iTAO3MutwM
オプショナルチェイニング(optional chaining)演算子ですっきりと書く
オプショナルチェイニング：https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Operators/Optional_chaining
if(result) {
passwordErrorMessage && passwordErrorMessage.remove()
}
passwordErrorMessageが trueであれば passwordErrorMessage.remove()を実行する処理です。
これを
if(result) {
passwordErrorMessage?.remove?.()
}
と記述することができます。すっきり。
あらかじめboolean以外のfalsyな値がわかっている時はその値と比較する
const argErrorMessage = document.getElementById('errorMessage')
if(argErrorMessage) {
...
}
これを
const argErrorMessage = document.getElementById('errorMessage')
if(argErrorMessage !== null) { // この部分
...
}
このようにすることで null以外の falsyな値が渡ってこないのが明確になり、リーダブルになるとのことでした。
falsyな値として false, null, undefined, 0, "" (空文字), NaNなどがありますが、今回の場合には
document.getElementByIdを使っているので、返却される値としては
指定された ID に一致する DOM 要素オブジェクトを記述した Element オブジェクト、または文書内に一致する要素がなければ null が返ってきます。
そのため、あえて他のfalsyな値が存在するということを示さないために、
if(argErrorMessage !== null)
としています。
[JavaScript] あらかじめboolean以外のfalsyな値がわかっている時はその値と比較する
— フロントエンドエンジニア (@terrace_tech) May 14, 2021
※あくまでIMO pic.twitter.com/WAz5BNwcpI
パフォーマンス編
document.createDocumentFragmentを使う
初めて聞いたときには、なんですかそれは？状態でした。
例えばこんな記述をしていたとします。
const body = document.querySelector('body')
const ul = document.createElement('ul')
const books = ['webを支える技術', 'Web技術の基本', 'JavaScript本格入門']
books.forEach(book => {
const li = document.createElement('li')
li.textContent = book
ul.appendChild(li) // ここで毎ループごとにulへliを追加している
})
body.appendChild(ul)
注目していただきたいのはforEachの中でulに対してliを追加している部分です。
これはパフォーマンスの観点から望ましいコードとはいえません。これはulにliを追加した時点で一度コンテンツが再描画されるからです。この程度であれば問題ありませんが、もっと大量のデータを扱うことになると大変なことになります。
要するに、なかなか負荷が高いことをプログラムにさせているということになります。
そこで **document.createDocumentFragment**の出番です。
DocumentFragmentオブジェクトは「組み立てたノードを一時的にストックするための容器」です。実際にコードを見ていきます。
const body = document.querySelector('body')
const ul = document.createElement('ul')
const books = ['webを支える技術', 'Web技術の基本', 'JavaScript本格入門']
let listFragment = document.createDocumentFragment()
books.forEach(book => {
const li = document.createElement('li')
li.textContent = book
listFragment.appendChild(li) // ここでDocumentFragmentオブジェクトに追加している
})
ul.appendChild(listFragment) // ulに対してDocumentFragmentオブジェクトを追加
body.appendChild(ul)
このように **document.createDocumentFragment**を使うことでパフォーマンスを最適化できます。
MDN : https://developer.mozilla.org/ja/docs/Web/API/Document/createDocumentFragment
配列操作・メソッド編
.map .forEach などの配列操作をするメソッドを使い分ける
slideImages.images.map((image, index) => {
const li = document.createElement('li')
imageListFragment.appendChild(li)
slideState.images.push(img)
})
このように .mapを使って配列操作を行なっていたのですが、本来 .mapは与えられた関数の処理をすべての要素に対して呼び出して、 **新しい配列を生成する**ためのメソッドです。
そのため、 .mapを知っている人からすると「新しい配列は？」となってしまい、リーダブルではありませんし、使い方を間違っています。
このコードの場合、特に新しい配列を生成する必要はないので、 .forEachが適切でした。
目的にあったメソッドを使う
const users = [
{
name: 'aaaa',
password: 'Yamamoto0000'
},
{
name: 'bbbb',
password: 'Yamamoto1111'
}
]
function checkSubmitData(inputUserData) {
const result = users.some(user => {
user.name === inputUserData.name && user.password === inputUserData.password
})
return result
}
フォームに入力された name と passwordが存在するかを見つける処理です。
このとき、 .someを使って実装していました。
とレビューをいただき、ハッとしました。
ユーザが存在するかどうかを確認したかった。のであれば .findのほうが適切ですよね。
.someは true を返す要素が見つかるとtrue を返すのに対し、
.findは **true を返した要素の値を返す。**という違いがあります。
some : https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Array/some
find : https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Array/find
その他
引数の順番や数を意識する
これはIMOだったのですが、自分が気になったので質問してみたものです。
先生の場合は 、 必須な値・必須な関数・オプション の順で引数を渡しているそうです。
ちなみに僕の場合は、処理内で出てくる順に引数に渡すようにしていました。
また、引数の数は3つまでが望ましく、それ以上引数がある場合には
・関数内での処理が多いので、処理を切り出すか別の方法で書けないか考える
・引数をオブジェクトにしてまとめる
が必要だと教えていただきました。
#まとめ
ここに書いたこと以外にももっと細かいレビューも頂いていますが、書ききれないのでよく見るレビューをまとめてみました。
ただ記事をみて学ぶのと、自分で書いて、レビューしてもらって、修正して、またレビューもらって...を繰り返すのとでは、定着率が全然違うなということがよくわかりました。やっぱり手を動かしたほうが勉強になりますね。
#紹介
私が所属している「もりけん塾」は、もりけん先生が運営するJavaScriptに特化したコミュニティーです。
フロントエンドエンジニアになりたい方にむけて、先生が道標となって初学者が迷わないように導いてくれます。コードの書き方、自走力を身に着けるにはとても良い環境です。
毎月1日に募集をかけていたのですが、ここ最近は募集かけなくても入塾したいとDMがくるそうです...!!（大人気）