はじめに
チームのメンバーが増えてレビューする機会が増えてきました。
それに伴って、これまでは、ざっとPRを見渡して、気になる点があれば都度コメントするという形でやっていましたが、これからはレビューする側もされる側も効率良くやらないと、スケジュールが押してしまうし精神的にも辛いしで、良くないなと思うようになりました。
なのでこれを機に、一度レビューの観点を整理しておこうと思ったのが、この記事を書くに至った経緯です。
まずは一般的なレビューの観点とか色んな方のレビューの観点はどういうものなのかを調べてみたのと、これまで受けてきた先輩のレビューや実務を通して学んできた事とを照らし合わせてみました。
しかし全部の観点となると量がかなり多くなってしまうので、今回は、
細かいレビューの前に最初に意識してレビューしたい事
という観点に絞りました。
- 意識すればすぐ着手できる項目
- コードが読みやすくなる項目
- レビューがしやすくなる項目
これらを記載しています。
前提
最近フロントエンド側のレビューを行う事が多いです。弊社のフロントエンドにおける使用技術がVue.jsである事から、これらを例に用いた記述が多めとなっています。
普段TypeScriptを用いていますが、記述量を最小限にするためJavaScriptで記載しています。
命名規則とかタイポとかの基本的な事はもちろん意識すべきなのですが、今回はそれらについては触れていません。
1つの役割ごとに関数分割しているか
1つの関数がエディタの表示内に収まっていない..そんなコードを見た事・書いた事があるのではないでしょうか？
処理を見る前に、まず見やすい状態になっていないと、ちゃんとレビューができない可能性があります。なので、一番最初の観点として記載しています。
ここで意識したい事は、
1つの関数に複数の役割を持たせていないか？
1つの関数が受け持つ処理の最小単位をもう少し小さくできないか？
という事です。
以下はかなり単純な例です。
「面積を計算する」「円周を計算する」「面積と円周を足す」という三つの役割を一つの関数内で行なっています。
// before
const calcCircleAreaCircumferenceSum = () => {
const circleArea = A * A * 3.14;
const circleCircumference = 2 * A * 3.14;
return circleArea + circleCircumference;
}
1つの役割ごとに関数分割し、1つの関数が受け持つ処理の最小単位を小さくします。
// after
const calculateCircleArea = (radius) => {
return radius * radius * 3.14;
};
const calculateCircleCircumference = (radius) => {
return 2 * radius * 3.14;
};
const calcCircleAreaCircumferenceSum = () => {
return calculateCircleArea(A) + calculateCircleCircumference(A)
}
こうする事で、その関数で何をやっているのかわかりにくい、テストが書けない、デバックし辛い、といった現象をできるだけ無くせると思います。
条件式が複雑になる場合は、条件式自体を関数化しているか
次は、条件分岐に関してです。
条件分岐は、処理が複雑になる代表例だと思います。
以下は条件式に || 演算子が使われている場合です。
// before
if (A.width > B.width || A.height > B.height) {
// ..existing code..
}
条件式が長く、意味もわかりづらいので、関数化します。
// after
const isALargerThanB = (A, B) => {
return A.width > B.width || A.height > B.height
}
if (isALargerThanB(A, B) {
// ..existing code..
}
条件式が明確に名前付けられ、その目的がより明確になります。
条件分岐内の処理が複雑になる場合は、それを関数化しているか
functionAの中に以下のような条件分岐があったら、これだけで見づらく感じるのではないでしょうか。
// before
const functionA = () => {
// ..existing code..
if (checkAB(A, B)) {
const circleArea = A * A * 3.14;
const circleCircumference = 2 * B * 3.14;
return circleArea + circleCircumference;
} else {
const circleCircumference = 2 * A * 3.14;
const circleArea = B * B * 3.14;
return circleCircumference + circleArea;
}
}
一つの役割ごとに関数分割します。
// after
const calculateCircleArea = (radius) => {
return radius * radius * 3.14;
};
const calculateCircleCircumference = (radius) => {
return 2 * radius * 3.14;
};
const functionA = () => {
// ..existing code..
if (checkAB(A, B)) {
return calculateCircleArea(A) + calculateCircleCircumference(B)
} else {
return calculateCircleArea(B) + calculateCircleCircumference(A)
}
}
条件分岐内のコードが1行ずつになってスッキリし、functionAのコードの見通しが良くなりました。
上記例は、そこまで複雑な処理ではないですが、それでも違いを感じます。
DOMのイベントやrefなどの算出プロパティに依存しない単純なロジックはvueファイルから切り出し、外部jsファイルに定義して、そこからimportしているか
直接vueファイルのscriptタグ内にロジックを記載した場合は以下
<script>
// before
const calculateCircleArea = (radius) => {
return radius * radius * 3.14;
};
const calculateCircleCircumference = (radius) => {
return 2 * radius * 3.14;
};
const functionA = () => {
// ..existing code..
if (checkAB(A, B)) {
return calculateCircleArea(A) + calculateCircleCircumference(B)
} else {
return calculateCircleArea(B) + calculateCircleCircumference(A)
}
}
</script>
外部jsファイルにロジック定義し、そこからimportした場合
export const calculateCircleArea = (radius) => {
return radius * radius * 3.14;
};
export const calculateCircleCircumference = (radius) => {
return 2 * radius * 3.14;
};
<script>
// after
import { calculateCircleArea } from '@/modules/sample.js'
import { calculateCircleCircumference } from '@/modules/sample.js'
const functionA = () => {
// ..existing code..
if (checkAB(A, B)) {
return calculateCircleArea(A) + calculateCircleCircumference(B)
} else {
return calculateCircleArea(B) + calculateCircleCircumference(A)
}
}
</script>
切り出す事で以下のメリットがあると考えます。
- 複数のcomponentで使用するロジックの共通化ができる事に気づきやすくなる
componentに関数を記載していると、意外と共通化できる事に気づきにくいのと、気づいたとしても、後から外部に関数を切り出すのが面倒なので、それならば最初からやっておきたいものです。
- Vue test utilsを経由したテストではなく、シンプルにJestやVitestを使って単体テストが書ける
テストが書きやすいという事は、品質が良くなる事に繋がるのですが、詳しくは こちらをご覧ください。
また、このレビュー観点は、Composition APIの「ロジックの抽出と再利用」の考え方に基づいています。詳しくは、Composition APIリファレンスをご覧ください。
実装の意図がコメントで記載されているか
これまで述べてきた事がコードに反映できていれば、一つの関数を見れば、処理の「内容」についてはわかりやすい状態になっていると思います。
しかしこれでも尚、なぜこの処理を行なっているか？という「意図」については、わかりにくい場合があります。
そういった時は、コメントで処理の意図が記載されていると、実装者以外が見た時に理解しやすいです。
先ほどのコードを例にします。
条件分岐の上にコメントを追加しています。
<script>
import { calculateCircleArea } from '@/modules/sample.js'
import { calculateCircleCircumference } from '@/modules/sample.js'
const functionA = () => {
// ..existing code..
// AとBをチェックし計算を出し分けて、画面に値を表示するという仕様のため <-- コメント追加
if (checkAB(A, B)) {
return calculateCircleArea(A) + calculateCircleCircumference(B)
} else {
return calculateCircleArea(B) + calculateCircleCircumference(A)
}
}
</script>
レビュアーが仕様を知らない場合や、仕様がわかりにくい場合、未来の自分自身がコードを見る場合を想定して考えてみます。
checkABの処理内容や、条件内の処理内容に関しては関数を見ればわかりますが、なぜこれらの処理をやっているかの意図がわからない事があります。
そういった場合、意図が記載されているとわかりやすいので、レビューの観点に含めています。
最後に
これを書きつつ、私も実装する際に気をつけないと..と思っています。
これから引き続きレビューをする機会が増えるでしょうし、ある程度これらの観点について、レビューされる側の方々にも知っておいてもらう事で、円滑に開発が進むようになったら良いなと思います。