こんにちは。りまりま団のもふもふです。2018年最後の出社日でした。コミックマーケット！やるぞ！！
入社からの半年間でコードレビューで指摘されたことのまとめというエントリをご存知でしょうか。業界未経験で入社された方がコードレビューで指摘されたことをまとめて記述した良エントリーです。
このように指摘されたことをまとめておくと、Pull Requestボタンをポチッとする前に見返しておくことができていいなーと思ったので、私もやってみることにしました。ちなみに社会人4年目ですが、本格的にWebアプリケーションのプログラミング + チーム開発を始めたのは今年の8月からです。このタイトルは嘘じゃないということですね。1
コーディング全般に関して
利用されていないものは削除する
- 利用されていない変数・importしたものを残した状態でPull Requestを送らない
これは何回も指摘されてしまいました…。試行錯誤した結果/仕様変更対応時に、不要になった変数やimport文が残ったままPull Requestを送らないようにしましょうという内容です。
Visual Studio Code（以降VS Codeと表記）を利用しているのですが、自動でimport文をしれっと記載してくれる機能があります。これでimportされたものに気づかずPull Requestを作成→レビューで指摘されてしまっていました。言語はTypeScriptです。 2
自動でimport文をやめるという設定があるのだろうなと思ってはいるのですが、これを切る方法がわからないので、目視で余計なimport文がないか確認しています。もっといいやり方があるのではと思いますが、みなさんどうしているのでしょうか…？
対策
git diffで差分のまとめを確認してからgit commitする習慣をつける- Pull Requestを送信する前にGitHub上の
Files Changedで余分なものが入っていないか確認する
省略記法の利用は他の人と足並みを揃える
- 他のファイルで省略記法が利用されている場合、自分の成果物も省略記法を利用する
Vue.jsには省略記法が存在します。例えば、v-bind:xxx='yyy'は:xxx='yyy'と記載できます。自分はAというコンポーネントを開発するときにv-bind:xxx='yyy'という書き方をしていました。しかし、他のコンポーネントでは:xxx='yyy'という書き方を利用しているものが大半でした。
このような場合、書き方を他の人に合わせましょう。という指摘を受けました。CSSのスタイルはスタイルガイドを作って統一しましょう。でないと破綻します。3 というものに通じているのかな、と思います。コードの保守性を保つためには表記が統一されていた方が良いからなのだ、と捉えています。違う書き方だとどちらにあわせれば良いのか迷ってしまいますからね。
既に開発が進んでいるチームにアサインされたときは、スタイルガイド・暗黙的なスタイルのつけ方がないかを確認して合わせるようにしようと思いました。心配なときは「XXを参考に〇〇と書きました。この記法でいいですか？」とPull Requestや口頭で確認するようにしています。
対策
- あらかじめスタイルガイドを導入しておく
- 自分の担当外の実装に目を通し、どのような記法が利用されているか確認して合わせる
- 心配だったら（レビュー時などに）質問する
Boolean型のデータが入る変数名
- Boolean型のデータが入る変数名は"それっぽい"名前をつける
v-ifでコンポーネントの表示を切り替えたいから、状態を切り替えるための変数を作ってv-ifにわりあてよう！名前はコンポーネントの名前aにしよう！…という思考回路で実装を行いました。すると、aという変数名は違和感があるからshowAみたいな名前に変えてね、と言われてしまいました。
なぜこの指摘を受けたのか、かの有名なリーダブルコードを読み返して考えることにしました。4
今回のケースは、第2章の『明確な名前を選ぶ』と第3章の『ユーザーの期待に合わせる』の改善点に該当するなと思いました。aという変数名からは、aに関する何かで利用されることがわかります。しかし、それ以上の情報はありません。また、他の表示切り替え系の実装を見るとis〜・show〜・active〜といった名前が多く使われていました。aという命名はどの法則にも当てはまりません。
こういう命名のセンス？は経験値に比例して上がっていくのだと思います。指摘されたら直し、同じ注意を受けないようにするには？と考えるようにしました。慣れてきたら他の実装（リポジトリ外のコードも含む）を参考により良い命名を検討するようにします。合わせてcodicを利用して表現の幅を広げる、という地道な対策で進めるしかないように思います。
対策
- 他の実装を参考に鍛錬する
- 命名サイトを利用する
メソッドはなるべく分ける
- 処理AとBのように内容が異なる処理はAメソッド・Bメソッドを作成する。1つにまとめたりしない。
A 値を作る処理 B Aの値を使って引数を変更し$emitを呼ぶ処理
これはリーダブルコード11章の『一度に1つのタスクを適用する』に当てはまる事例です。始めはAとBの処理を続けて実行するCメソッドを作成していました。しかし、今後Aの値を作る処理だけ再利用したい場合に困ってしまいます。分割できそうな処理は分割する、これは慣れるまでは意識してやるようにしないとすぐコードが肥大化します(戒め)。
原因について考えてみました。
- そもそも「切り出す」という発想がない
- 「切り出すと動かない/動かなくなるのでは」と不安になる（＝やらない）
- 処理を書いて力尽きる
始め・2番目原因はメソッドの呼び方を知ることで対策できそうです。冷静に考えればメソッドを作る意味ないじゃん！なのですが、初めてのプログラミングだと与えられた課題/仕事を進めるので精一杯です。とりあえず動かさないと！で頭がいっぱいなのでメソッドの役割は頭の中から転がり落ちていきます。
オブジェクト指向についての話、のように高級な話題を受け入れるだけの容量は残っていないのです。関数型言語はやったことがないのでわかりません。考え方も違うのでしょうか？とにかく、大半のメソッドは他から呼び出せるんだと教えてあげてください。あとは方法を調べればいいだけです。
最後に関しては、全て書き終わった後に休息するくらいしか対策が思いつきません。ちょっとそこまで飲み物を買いに行ってリフレッシュするのがいいと思います。そういうのができない現場では働きたくないですね…。レビューに出すときに「ここが心配です」と申し送りしておく、とかでもいいかもしれません。
対策
- メソッドは再利用できることを知る
- 内容が違う処理はメソッドを分ける
- 心配なら申し送りして確認してもらう
ウィンドウのリサイズ対応
- 画面幅が変わる可能性があるHTML要素には、画面幅の変更に追従できるようなCSSを割り当てる
今回はBtoB向けのWebアプリケーション開発だったので、ディスプレイ幅やスマートフォン対応は考える必要がありませんでした。しかし、ボタンを押すと画面がｻｯと横から出てくる場面があったので、そこで画面幅が変わる可能性がありました。
こういう点、視点がなければ配慮・考慮もせずにレビュー依頼してしまうんだな…と思いました。
対策
- 単位を
%指定にする calc()を利用するvh・vwを利用する
CSSに関しては手数をどれだけ知っているかで表現の幅が広がるように思います。色々手数を増やしていきたいですね。
出来るだけJavaScriptでスタイルを操作するのはやめる
// testクラスのCSSを条件に応じて変更したい場合 <template> <div class="test">aaa</div> </template> // （Vue.jsなら）このようにするのではなく <template> <div class="test" v-bind:style="'color:' + red" v-bind:class="{active: isActive">aaa</div> </template> // こうする <template> <div class="test" v-bind:class="{active: isActive}">aaa</div> </template> <style> .active{ color: red; } </style>
Vue.jsのようにリアクティブなものに限るかもしれないのですが…。「Aの条件のときは文字色を赤にする」ような場合、JavaScript側ではクラスのバインディングのみ行うようにし、バインディングされたクラスに対してCSSを利用するようにした方が再利用性も高く良いと指摘を受けました。
言われてみると、こちらの方が幾分も見やすくなりますし、色を修正したい！となった場合にHTMLのテンプレートに手を入れる必要がなくなります。
条件分岐の条件に定数を利用するのは避ける
- 赤色が入っていたらRの処理、青色が入っていたらBの処理…としたい場合
×赤色が入っていたらRの処理、青色が入っていたらBの処理を書く ○状態管理用の変数を作成し、変数の値を条件分岐に利用する
「赤色が入っていたらRの処理、青色が入っていたらBの処理」で実装すればわかりやすいやろ、と思っていたのですが、赤色を黄色に変えたいとなったときに大量の修正が発生するので困ってしまうという指摘を受けました。色や定数のように、これ以降、処理の内容と直接関係ない部分をロジックの部分に利用するのは避けるようにしました。
対策
- 変数や定数を条件分岐に利用しない
- 状態を管理する変数を利用できないか検討する
Docker
dockerignoreの利用
- dockerignoreを利用して、node_modulesなどのライブラリ関連のディレクトリは無視できるように設定する
イメージのビルドをする際Dockerfileが存在するディレクトリ全体を圧縮してしまうため、何もしないとnode_modulesなどもDockerイメージの中に入ってしまい、ライブラリの依存関係でリビルドが必要になってしまう事象がありました。個人の環境に依存するようなものはdockerignoreに追加したほうが良いです。gitignoreのように、テンプレートを探して適用してもいいのかなと思いました。
対策
- ライブラリ用のディレクトリは
dockerignoreにあらかじめ入れておく - セットアップ時にテンプレートがないか探してみる
ライブラリ
package-lock.jsonの扱い
- npm installしたときにpackage-lock.jsonへ差分が出てしまった場合、その差分をコミットしないようにする。
package-lock.jsonのなかで^（キャレット）が付いているパッケージは明示的にパッケージのバージョン管理をしています。これを変更してしまうといつの間にかライブラリのバージョンが変わってしまって意図せず不具合が発生してしまう可能性があるため、変更があればgit resetなどで元に戻してコミットするようにしました。
パッケージ管理はnpmで行っていました。npm installしたときに^が付いていてもpackage-lock.jsonが更新されてしまうときがあるのですが、これ何とかならないものでしょうか…？npmの設定周りをどうやって調べたらいいのかが分からないんですよね。npmのドキュメントのどの部分を中心に見れば良いのかが謎です。今はgit resetやstashを使って変更されてしまった部分を変更前に戻して対応しています。
対策
- lockファイルにキャレットが付いていた場合、ライブラリのバージョン更新時にキャレットが外れていないか確認する
その他
指摘されたけど対策がわからないもの
指摘されたけれど、再度注意されないようにするにはどうしたらいいのかわからない事項もあります。来年には解決してるといいんですけど…。
- getterをいつ利用するか
- 「値の取得はgetterで」と言われたけれど、getterを利用せず値を取得する場面もある。結局いつgetterを利用すればいいんだろう？
総括
プログラミング初心者向けを謳う記事はこういうことを書いて欲しいです。文法とかは学習サービスがあり溢れてますし、勉強法なんて人それぞれなんです。そういうのはもう結構なんです。
こういう言語化されていないけどマナーとしてやるべきことや、処理がうまくいかないときの考え方・調べ方・調べた結果をどうやって参照するか…がもっと広く共有されればみんな幸せになるのではと思います。そういう意味だと自分の書いた『ログと情報をレッツ・ラ・まぜまぜ！～ELK Stack で作るBI環境～』5というKUSO同人誌のプログラミング言語版みたいな本が出てくればいいなと思いました。
来年も振り返りができるように記録を残しつつやっていきたいと思います。
