こんにちは、YOUTRUSTでWebエンジニアをしている今井（YOUTRUST / X）です。本日は普段私がいただいているレビューの一部をご紹介します。
6月に入社してから、創業エンジニアのやまでぃさんのもとで研修を行なってきました。本当にありがたいことに沢山のレビューをいただき、学びしかない研修期間でした。今回ご紹介するレビューは、その中のほんの一部ですがご紹介させていただきます。
（別の機会に、研修の内容についてもブログで書きたいと思います。）
以下が今回ご紹介するレビューの内容になります。 読んでいただくと分かると思うのですが、プログラミング言語の文法など技術的なレビューは少ないです。それよりも「可読性の高いコードとは？」「保守性を高めるコードとは？」といった観点のレビューを多くいただきました。
技術的なスキルは後々付いてくるので、それ以上にコードを読む人の気持ちを想像して、「読みやすいコード」「10年後に自分(または他のエンジニア)が読み返しても分かりやすいコード」を書くことの重要性を学んできました。
変数名は意味が伝わることを最優先に
レビュー前
チャットルームの合計数を意味する変数名としてtotalChatCountという変数名を命名。
const totalChatCount = //省略
レビュー後
totalChatCountでは、少し意味が伝わりづらいかもというレビューをいただきました。なぜならtotalChatCountでは「ルーム数？、それとも、メッセージ数？」と迷う余地があるからです。
一目でルーム数と分かるように、totalChatRoomCountに修正しました。
const totalChatRoomCount = //省略
分かりやすさ最優先で
レビュー前
スカウト返信率を計算するための関数をフロントエンドで定義をしました。その際に引数は値だけを指定するように定義していました。
export const replyRate = (totalChatRoomCount: number, repliedCount: number) => { return /* ロジック */; }; replyRate(100, 30) // どっちが分母分子か分からない。順番間違えていたとしても気づきづらい。
レビュー後
原則キーワード引数を受け取るようにというレビューをいただきました。レビュー前のように値のみを指定した場合、「第一引数と第二引数のどっちが分母だっけ？」と迷う余地が出てくるからです。
少しコードは長くなりますが、分かりやすさを最優先にコードを修正しました。
const replyRate = ({ totalChatRoomCount, repliedCount }: { totalChatRoomCount: number; repliedCount: number }) => { return /* ロジック */; }; replyRate({ totalChatRoomCount: 100, repliedCount: 30 }) replyRate({ bunbo: 100, bunshi: 30 }) // ちょっと格好悪いけど、分かりやすい
バックエンドとフロントエンドで同じロジックを持たないように
レビュー前
管理者用ツール（Admin）の中で、ユーザーの権限によって閲覧可否が変わるページがありました。私はその際に、「権限Aと権限Bのユーザーの場合、ページを閲覧できる」という認可のロジックをバックエンドとフロントエンドの両方に記述をしていました。
レビュー後
バックエンドとフロントエンドで同じロジックを持たないようにというレビューをいただきました。将来的にロジックが変更された場合に、バックエンドとフロントエンドの両方のコードの修正する必要になってしまうからです。
また両方にロジックがあることに気づかずに、どちらか片方の修正が漏れてしまう可能性が出てきます。（現状は権限Aと権限Bのユーザーしか閲覧できないページでも、将来的に権限Cのユーザーも閲覧できるように変更になるなど）
認可のロジックはバックエンドのみに記述して、フロントエンドにはAPIで閲覧可否のフラグを返すように修正しました。
早すぎる共通化
レビュー前
スカウト返信率を計算する関数を実装し、他のコンポーネントでも共通利用できるディレクトリに配置しました。
export const replyRate = () => { return /* ロジック */; };
レビュー後
共通化するには少し早いというレビューをいただきました。今回の場合、「他のファイルで上記の関数が利用されるケースはまだ無さそう」というのが大きな理由でした。
またモノによっては、共通化した後に仕様が変わる可能性もあります（今回のスカウト返信率の計算ロジックは大きく変わらないかもですが）。「早すぎる共通化」は将来仕様変更が起きた際に、むしろ修正コストがかかり必要以上に手間がかかってしまう可能性があります。
そのため今回は一旦共通化を行わず、この関数を直接使用するファイルだけに配置しました。
（そもそも、replyRateはスカウト返信率の計算専用の名前になっているので仮に共通化するならば、もう少し抽象度の高い命名をした方が良いというレビューもいただきました）
「どのように見せるか？」を考えるのは、フロントエンドの責務
レビュー前
ユーザーさんのスカウト数などを取得するためのAPIを作成しました。そのAPIの中で、スカウト数、返信数に加えて、スカウト返信率もバックエンドで計算して、APIで返すように実装しました。
properties: scouting_count: # スカウト受信数 type: integer scouting_and_replied_count: # スカウトに対する返信数 type: integer scouting_reply_rate: # 返信率 type: string
レビュー後
スカウト返信率はフロント側で計算するようにとレビューをいただきました。 「どう見せるか？」「小数点以下の何桁まで表示するか？」はフロントエンドの関心ごとだからです。 例えば、「デスクトップ版では小数点以下3桁まで。モバイル版は画面が小さいから整数値。」のように柔軟性のある実装が可能になります。
なので今回は返信率を計算するための材料だけをフロントに渡すように修正しました。
properties: scouting_count: # スカウト受信数 type: integer scouting_and_replied_count: # スカウトに対する返信数 type: integer # 返信率はAPIで返さずに、フロントエンドで計算するように修正
適切な箇所で改行しよう
レビュー内容
普段オフィスでは大きなモニターで作業をしているので、1行が長いコードも、ついそのままにしてしまうことがありました。
ですが全員が大きなモニターで作業をしている訳でもないですし、自分以外の人が読むことを想像すると、適切な改行が大事です。適切な箇所が何文字なのかは一概には言えないと思うのですが、
- 「100文字を超えると改行をした方がいいかな」
- 「メソッドチェーンを書くときは並列的な概念が分かるように、小まめに改行しよう」 という意識で取り組んでいます。
小さい端末やGitHubで折り返しされない文字数という感覚も学びました。（何よりシンプルに長い文章は読みづらいですよね、、）
最後に
まだまだ沢山の勉強になるレビューをいただいたので、ご紹介したいレビューはもっとあるのですが、今回はここまでにしようと思います。私はWebエンジニア1年目で、入社した当初は、レビューでは文法など技術的な指摘を多くいただくと思っていました。
ですが、研修の中でいただいたレビューの8割以上（もしかしたら9割以上）は今回ご紹介したような、「可読性の高いコードとは？」「保守性を高めるコードとは？」といった観点のレビューでした。「コードを読む人の気持ちを想像して、読みやすいコードを書く」ということは、言葉にするとシンプルですが、本当に難しくとても苦労しました。
これからも「5年後、10年後に見返しても分かりやすいか？保守しやすいか？」ということを意識してコードを書いていこうと思います。
最後まで読んでいただきありがとうございました！ 株式会社YOUTRUSTでは、エンジニアを絶賛募集中ですのでご連絡お待ちしております！
