この記事はBASE Advent Calendar 2019 の2日目の記事です。
こんにちは！BASEでFrontend Groupに所属している三佐和 です。主にネットショップ作成サービス「BASE」のフロントエンドを担当しています。
2016年に未経験からBASEでインターンを開始し、その後正社員として入社しました。当時は主にLP などのマークアップや管理画面のUI に関わる細かい修正などを担当しており、使っていた技術は主にHTML, CSS, jQuery で、LP で使うような簡単なアニメーションの実装しかしたことがなかったのでJavaScript の知識はほぼ皆無でした。
今回のブログでは、リーダブルコード を読みながら、今年1年間書いてきた自分のコードを振り返ってみたいと思います。
今年主に取り組んできたこと
昨年、ショップオーナーさんが使う管理画面のフルリニューアルプロジェクトが本格的に始動しました。 （プロジェクトについてのお話は、昨年のアドベントカレンダーでデザイングループの早川が「BASE」の管理画面リニューアルプロジェクトのこれまでとこれから という記事を書いています。）
それと同時期にフロントエンドチームが新たに発足し、インターンの頃から「アシスタントデザイナー」だった肩書きが「フロントエンドエンジニア」になりました。
（当時の心境）
「アシスタントデザイナーといいつつもデザインはできないし、自分は一体何者なんだろうって思ってきたけど、今日から私はフロントエンドエンジニアなのかー！嬉しい！」
Vue.js + TypeSrcript の新しいアーキテクチャを採用することになり、Vue.js の勉強が必要だと気付いた私… 勉強しながら、アウトプットとしてデザイナー向けにVue.js 勉強会 Vue Boot Camp を開催したりしました。
そして今年、管理画面リニューアルプロジェクトの中のひとつである注文管理画面のフロントエンド側実装を担当することとなりました。
（当時の心境）
「Vue.js なんとなく触ったりしたけど、これがWeb アプリケーションの中でどう書かれて、どう動くのか全然予想がつかない…こんな大きなプロジェクトにアサインされて大丈夫なのか…こわい…」
でもとりあえず、やっていき
2月に爆誕させてから、模索しつつ書いてきたコードたちを振り返ってみたいと思います。
コードを振り返ってみる
ガード節を使って正常系と異常系を明確にする
全体の状態を管理するために HashMap を使用していましたが、HashMap は中身の変化を Vue が監視することができないため、代わりのキーとして updated を監視対象に含めることにしました。
修正前
class extends Vue { // ... ordersAt(page: number): ReadonlyArray<Order> | null { if (this.updated) { return this.allOrdersMap.get(page) || null } return null } updateOrders() { // ... this.updated = + new Date() } // ... }
レビュー
あとで直しにくいし、読みにくくなるので、最後のreturn が異常系になるような書き方はあまりしないほうがいいです
ガード節という考え方を使って
typescript if(!this.update) { return null }を先に書くといいですよ
修正後
ordersAt(page: number): ReadonlyArray<Order> | null { if (!this.updated) { return null } return this._allOrdersMap.get(page) || null }
updated は Vue の都合のためのコードなので、それが存在しないのは明確に処理の実行が不可能になります。今回の場合は監視さえできればよいので、guard の方が適切です。
条件分岐を読みやすくする
条件によって表示が切り替わる処理です。期間の指定をしているかどうかで表示を変更します。
修正前
<template> // ... <p v-if="!condition.from && !condition.to"> {{ __('すべての期間') }} </p> <div v-else> <p>{{ format(condition.from) }}</p> 〜 <p>{{ format(condition.to) }}</p> </div> // ... </template>
レビュー
こういうケースは
not and notよりも、orのみの方が分かりやすいと思うので、実際に期間を出す方をfrom or toとした方が良い感じがしますね
修正後
<template> // ... <p v-if="condition.from || condition.to"> <p>{{ format(condition.from) }}</p> 〜 <p>{{ format(condition.to) }}</p> </p> <div v-else> {{ __('すべての期間') }} </div> // ... </template>
!A and !B は最後まで読む必要がある上に、否定なので基本的に読みづらく、とにかく期間が指定されていれば表示を変えるのだから「どちらかが指定されていたら」という条件の方が自然です。
意図を伝える
key な文字列となっている prop を実際の表示のために日本語に変換する処理です。
修正前
class extends Vue { // ... get paymentInfoTitle() { if(this.isCreditCard) { return 'クレジットカード決済' } else if (this.isBank) { return '銀行振込' } else if ... { // ... } return '' } // ... }
レビュー
switch で書いた方が意図は通じやすいかなと思った
修正後
class extends Vue { // ... get paymentInfoTitle() { switch(this.$props.payment) { case 'credit_card': return 'クレジットカード決済' case 'bank': return '銀行振込' // .... } } // ... }
if-else ... としていくよりも、switch でこのうちどれかになるという網羅性がある方が可読性も高そうです。
一度に一つのタスクを行う
選択されている検索条件をstring として表示させるための処理です。
修正前
class extends Vue { // ... get searchConditionStatus() { return this.searchCondition.status.map(x => { return orderStatuses.find(status => { return status.key == x })!.name }).join(', ') } // ... }
レビュー
map などの関数型インターフェイスを利用する場合は一個一個の関数の処理を小さな単位で切り出す方が読みやすいです
今この関数は「statusをオブジェクトに変換して、nameを取り出す」という処理をしていますが、それを二つに分ける感じ
修正後
class extends Vue { // ... get searchConditionStatusLabel() { return this.searchCondition.status .map(x => { return orderStatuses.find(status => { return status.key == x })! }) .map(x => x.name) .join(statusSeparater) } // ... }
小さい単位で処理を切り出すことで、概要が把握しやすくなりコードが読みやすいものになります。
プロジェクト固有のコードから汎用コードを分離する
string として変換済みのお届け時間帯を、人が読める形式に戻す処理です。このような処理は、他の箇所でも登場する BASE ではお馴染みのものです。
修正前
class extends Vue { // ... deliveryTime(time: string) { const startTime = time.slice(0, 2) const endTime = time.slice(2, 4) if(time == '0812') { return '午前中' } return startTime + '〜' + endTime + '時' } // ... }
レビュー
このあたりの変換は
timeslot-to-stringのような機能として切り出してもよいかも
修正後
import { timeslotToString } from '~/util/timeslot' class extends Vue { // ... get deliveryTime() { return timeslotToString(this.orderDetailDeliveryDate.delivery_time_slot) } // ... }
// ~/util/timeslot.ts export const timeslotToString = (time: string) => { const startTime = time.slice(0, 2) const endTime = time.slice(2, 4) if(time == '0812') { return '午前中' } return startTime + '〜' + endTime + '時' }
Vue(View) から処理を抜き出すことで、他の箇所からも参照できる関数になりました。また、Vue での関心ごとが減り、コードが少なくなりました。
子コンポーネントの中からの視点でイベント名を決める
子コンポーネント内でチェックボックスを選択し、submit ボタンで$emit し、親コンポーネント内でモーダル（hoge-modal）を開く処理をします。
修正前
// 子コンポーネント <template> //... <bbq-button @click="submit" >submit</bbq-button> //... </template> <script> //... class extends Vue { //... submit() { this.$emit('show: hoge-modal') } //... } </script>
// 親コンポーネント <template> //... <子コンポーネント @show:hoge-modal="isHogeModalVisible = true" ></子コンポーネント> //... <hoge-modal :show="isHogeModalVisible" @cancel="isHogeModalVisible = false" > //... </hoge-modal> //... </template>
レビュー
コンポーネントを設計していく上で、このコンポーネントは親コンポーネントの存在を知らないと考えるのが基本です
そうなると子コンポーネントが出来ることは「submitされた」というイベントを送ることだけになります
子コンポーネントはイベントを受け取った親コンポーネントが何かをshowするかどうかを知りませんし、hoge-modalというものが存在することも知りません
このイベント名だと、具体的な親コンポーネントが存在する前提のイベント名になるんですが、その逆で子コンポーネントの中からの視点でイベント名を決めるとしっくり来る名前になりやすいです
修正後
// 子コンポーネント class extends Vue { //... submit() { this.$emit('submit') } //... }
// 親コンポーネント <template> //... <子コンポーネント @submit="isHogeModalVisible = true" ></子コンポーネント> //... </template>
子が意識することは「submit すること」だけなので、それに合わせてイベント名を変更しました。
リリースを終えて
無事に開発は進み、6月に注文一覧ページ、11月に注文詳細ページ をリリースすることができました。
意識していたこと
プルリクを作る単位を細かくする
ついつい「ついでに」これも直しておこう、あれも直しておこう、と細かい修正を含めてしまい、結果的に大きなプルリクを作ってしまっていた為、なかなか作業中のプルリクをレビューに出せずにいました。
ひとつのプルリクに含まれるファイルの差分が大きくなればなるほど、その分レビューしてもらう範囲も大きくなり、レビューコストも高くなってしまうので、なるべく小さな単位でプルリクを作り、開発速度を上げられるよう意識しました。
コミットメッセージにはそのコミットにおける修正理由を書く
リーダブルコードの第5章「コメントすべきことを知る」にも書かれているように、コードを読めばすぐに分かるような内容をコメントやコミットメッセージに書くのではなく、それぞれ、コードを読む人がそのコードを理解するまでにかかる時間を短くする為のコメント、どうしてこの修正をしたのかを伝える為のコミットメッセージを書くよう意識しました。
できるようになったこと
会話ができるようになった
プルリク内で、「コードを書いた意図を聞かれて、それに対して自分の見解を述べる」というのはごく当たり前な光景かもしれません。ですが、注文管理のコードを書き始めた頃の私はとにかく動くものを作る、という一心でコードを書いていたため、明確な意図などなく継ぎ足し継ぎ足しで書いていくうちにコードが出来上がっていたため、レビューでコードの意図を聞かれても、なぜそのようなコードになったのか、自分の言葉でうまく説明できずにいました。
Vue.js を書いていくうちに、コードを書き始める前に「今自分は何を（実装）したいんだっけ」と一度頭で整理してから手を動かすようになり、「これを書けば期待する動きが実装できそう」と予測しながらコードを書けるようになり、それからやっとレビューをもらったときにそのコードを書いた意図を説明できるようになった気がします。当たり前のことではありますが、うまくできなかったことができるようになったということは、私にとってとても大きな変化でした。
予測できるようになった
上記の通り、Vue.js を書くことに段々慣れてくると、一度頭の中で整理する習慣がつきました。その中で、「この書き方は冗長だな」とか「ここは共通化したほうがすっきりするな」などと思うようになり、コードを読む人にとって読みやすいコードや自分も実装するときに頭の整理がしやすいようなすっきりしたコードを書けるよう、段々意識するようになりました。
「これってどういう動きをするんだろう…？」とひとつひとつ確かめながらコードを書いていた頃に比べると、うまく動く予想も、きっとエラーが出るだろうという予想もどちらもできるようになり、開発がしやすくなったように思います。
エラーをきちんと読むようになった
予想しながら書いたコードを実行した時にエラーが出ると「うっ…」となります。ですが、エラーは焦らず落ち着いてよく読むと、解決するためのヒントが書いてあり、何もこわいものではないということを学びました。
例えば、よくあるエラーでいうと
(変数名) is not defined
これは「 (変数名) なんて変数、定義されてないよ！」というエラーなので、まずタイピングミスがないかを疑います。
Object is possibly null
これは「 null かもしれないオブジェクトに対する操作はできない」というエラーなので、null じゃないことを保証すればいいんだな、とコードを見直します。
JS の勉強を始めたばかりの頃は、エラーが出たときに真っ先にエラー文をコピペして検索し、同じような事象やそれに対する解決策がないかを探していましたが、どこの行で起きているエラーなのか、エラー文をよく読み、debugger や console.log() などを使用して、処理の流れを追いながらエラーの原因を探していくことが解決するための一番の近道だと気付いたことでエラー文を見ても冷静に対処できるようになったと思います。
まとめ
注文管理画面リニューアルプロジェクトが始動してから約9ヶ月間、ひたすらにコードを書き続けてきましたが、肩書きがフロントエンドエンジニアになったばかりの頃に比べて自分は何か少しでも成長できているのだろうか、フロントエンドエンジニアとしての仕事がしっかりできているのだろうか、と不安になることも多々ありました。
しかし、フロントエンドチームが発足してから徐々に人も増え、チームのメンバーが私の拙いコードを丁寧にレビューしてくれたり、質問や悩んでいることがあればいつでも聞いてくれるおかげで自分の弱点や改善点に気付き、時間はかかっているかもしれませんが、徐々にできることも多くなったと実感しています。
また、プロジェクトメンバーであるバックエンドエンジニアやデザイナーなど、プロジェクトに関わる人と同じ目線で相談事や提案ができるようになったことも自分の中では大きな成長でした。
プロジェクトメンバーの一員として、フロントエンド担当として、実装パターンが多岐にわたる注文管理画面の実装を完遂することができ、フロントエンドエンジニアとしての自信がつきました。次回携わるプロジェクトでも今回の反省や学んだことを生かして、開発を楽しみたいと思います！
明日はCorporate Engineeringをやっている加賀谷さんと基盤Groupの川島さんです！お楽しみに〜！
