Sansan Tech Blog

Sansanのものづくりを支えるメンバーの技術やデザイン、プロダクトマネジメントの情報を発信

新卒の私が受けたコードレビューを振り返ってみた

Eight 新卒

こんにちは、Eight事業部で Eight Career Design(ECD)のフロントエンド開発をしている藤野です。ECDとはEightを利用されている方のスキルや人脈といった情報にもとづいて、各企業が求める人材をリコメンドすることでより良い出会いを提供する、というサービスです。ECDについて詳しくは以下の記事にて紹介されています。

buildersbox.corp-sansan.com

私は2020年に新卒としてSansanに入社しました。入社前にも半年程インターンとして開発に参加していました。早く業務に慣れ、事業に貢献できるように日々受けたレビューを振り返り、成長できるように努めています。今回は私が受けたコードレビューの分類と、その振り返りについて書いていこうと思います。

コードレビューって何？

エンジニアとしてチーム開発を行う上で避けては通れない道、それがコードレビューです。コードレビューとはその名の通り、修正・実装したコードを他者がレビューすることです。コードレビューは、バグの検知だけでなくより良いコードの書き方を提案する場にもなっています。

業務ではGitHubを用いたコードレビューをしています。おおまかな開発フローは以下のような感じです。

  1. チーム内で実装箇所、懸念点の洗い出し
  2. 実装（ペアプログラミングなどを含む）
  3. GitHubでPR（プルリクエスト）を投げる
  4. レビューを受ける
  5. OKが出るまで修正する
  6. マージ！

今回はもらったレビューを分類して、どのような視点でレビューされているのかを再度理解し、改善する方法を探っていきます。

今までに受けたレビューを分類する

今までに私がGitHub上で出したPRの数はインターンを含めた計8ヶ月間で、合計86個でした。その中から、自身の尺度でレビューを大まかに分類すると以下の4つに絞ることができました。

  • コードの可読性について（10個）
  • 命名について （9個）
  • コンポーネントの責務について（6個）
  • PRの作り方について（6個）

これらの具体例を交え、自身が開発を行う上でどのような視点を持って開発・改善していくのかを考えます。

レビューを振り返る

その1.コードの可読性

コードの可読性はチーム開発においてとても重要なポイントです。コードが冗長で可読性が低くなると、今後私や私以外の人がこのコードを見た時、理解するのに時間を要することになります。また、一目で意味を理解できないのでバグを誘発することも考えられます。

事例: 複雑な条件式

function isPubliclyValid() {
    return this.isStatusPublic && (
        (isNil(this.startDate) && isNil(this.endDate)) ||
        (!this.isPreOpen && !this.isExpired)
    );
}

これは、データを公開して良いかを判別するメソッドです。まずデータがパブリックであること。次にデータを公開する開始日と終了日が指定されていない、もしくはデータが公開期間中であること。 この二つを満たす時にtrueを返します（言葉にしてもわかりにくい）。

レビュー

言いたいことはわかるのですが、ちょっと条件が複雑過ぎだと思います。

実装をしている私は仕様を把握していますが、レビューを行う人がそれを把握しているとは限らないことを理解していませんでした。振り返っている今でも、この条件式を読むのに時間がかかったので、可読性の面で問題があるのは明らかです。

改善するには？

改善例のレビューもいただきました。

()が2回ぐらいネストしたら怪しむ方がいいかと（もちろん例外はあります）。 これらが何を表しているのか、if文や変数定義などで分解したほうがいいかも?

const dateIsNotSet = isNil(this.startDate) && isNil(this.endDate);
const rangeIsValid = !this.isPreOpen && !this.isExpired;
return this.isStatusPublic && (dateIsNotSet || rangeIsValid);

やっていることは同じでも、変数に置いてどのようなことをしているのかを変数名で伝えることで、読みやすいコードになりました。また、もっと複雑なコードになった場合は、一つひとつをif文で書いて場合分けをする方法が良さそうです。

レビューを改めて振り返ると

  • 仕様を知らない人でも理解できるコードにする
  • 変数やif文を用いて処理を分けて書き、情報量を持たせて読みやすくする

が今後の改善アクションになりそうです。ただ、場合分けのためにifのネストが深くなったりすると、逆に可読性が下がるので、バランスのよいコーディングが求められると感じました。

その2. 変数の命名

変数の命名は、その変数が何をしているか、何に用いられているのかを一目で理解するためのものなので、適切に付けなければいけません。

事例1: 変数の情報量が少ない

const url = content.url;

事例1のレビュー

変数名の情報が少ないです。何のURLなのかを意味するようにして欲しいです。

変数の情報が不十分であるため、この変数がどのような場面で用いられるのかをイメージできないコードになっています。 命名は可読性の問題と通ずるものがあり、適切な命名でないとコードを読むのに時間がかかったり、バグの原因になったりします。

事例2: 処理に対して適切な命名でない

const validateAnswers = validate(answers);

事例2のレビュー

validateメソッドはvalidな配列を返すメソッドなので、 動詞であるvalidateを使うのは少し適切ではないと思いました。

単なる配列に対して、動詞であるvalidateを使っていました。 validateAnswersのままだと、validateからfunctionが返ってくるように受け取られる可能性があります。

改善するには？

情報量が少ないことに関しては、その変数がどのようなことをしているのかを考え、その内容から英単語に翻訳し変数につけるようにしました。例えばURLでも、公開されている募集要項のURLの場合であれば

const publishedRequirementURL = content.url;

とすれば、変数名は長くなってしまうものの、一目で何をしている変数なのか理解できるようになります。私の場合英語があまり得意ではないので、Google翻訳を片手に変数のキーワードで翻訳を行ってから、変数を命名するようにしました。他にもcodic.jpといった変数のネーミングをしてくれるサイトもあります。

処理に対して適切な命名でない場合ですが、主に変数は名詞、関数は動詞を用いることを心がけるようにしました。 先ほどの例では、validateAnswersは変数なので

const validAnswers = validate(answers);

のようにしました。命名する場合は翻訳をそのまま貼り付けるのではなく、名詞と動詞をうまく使い分けて命名する必要があります。

改めてレビューを振り返ると

  • 変数名に情報を持たせる(英語が苦手な場合は翻訳サイトなどを駆使)
  • 主に変数には名詞、関数には動詞をつける

が改善アクションになりそうです。実装するときには、ちゃんとレビュワーが理解できるように、変数に適切な量の情報を持たせることが大事だと感じました。

その3. コンポーネントの責務について

コンポーネントを作成する時に、そのコンポーネントがどのような責務を持つべきかを考えて実装する必要があります。責務がバラバラだと、どこでデータを管理しているのかコードを追いかけることが難しくなります。

事例: エラーハンドリング

Formコンポーネントを持ったページを実装する際、Redux上で保存されているエラーメッセージをページの読み込み時に消すようにしました。このとき、Formコンポーネントがマウント(描画)された時点で、一度エラーメッセージを削除するように実装しました。

const Form = () => {
    // マウント時に1度だけ実行される
    useEffect(() => deleteErrorMessage(), []);

    ...
}

レビュー

Formコンポーネントのマウント時にエラーを削除をしてしまうと、エラーが必ず消えてしまうコンポーネントになり、再利用性が低くなると思います。

Formコンポーネントが持つべき責務をあまり考えず実装していました。Formコンポーネントがエラーメッセージを管理し、削除するコンポーネントになってしまうと、他のページでこのコンポーネントを利用することが難しくなります。また、データを管理するところが分散してしまうので、次にコードを変更する場合に、どこでエラーメッセージが管理されているかを追うのが難しくなり、バグ等の原因になります。

改善するには?

レビューの通り、Formコンポーネントを利用しているPageコンポーネントのマウント時にエラーメッセージを削除すれば、再利用が可能になります。

const Page = () => {
    useMount(() => deleteErrorMessage(), []);
    return <Form />;
}

実装時に発見されたバグをただ潰すことだけではなく、そのコンポーネントが再利用される可能性や、そもそもこのコンポーネントの責務はなんなのかを考える必要がありました。

改めてレビューを振り返ると、

  • そのコンポーネントが何をするべきなのか
  • どのようなデータを保持するのか
  • 再利用される可能性はあるのか

を考えながら実装をしていくと良さそうです。

その4. PRの作り方

PRとは、私が実装した変更内容をレビューしてもらうためのものです。正しいコードレビューを受けるには、正しいPRを作る必要があります。

事例1: ダイアログの見た目を修正したPR

できるだけ画面が変わるPRにスクショを、動作が伴う場合にはGIFをつけてほしいです。

ダイアログの見た目が変更になる修正を行いましたが、変更前・変更後の見た目をPRに乗せていませんでした。 また、ダイアログのボタンを押した場合のロジックも変更しましたが、どのような動作になったのか視覚的には載せていませんでした。

事例2: クリックしたときのアクティビティログを追加したPR

ブラウザの開発者ツールで通信ができていることを明記した方が親切かと思いました。

ボタンをクリックしたときに、実際にアクティビティログが送られているかのエビデンスがPR内に書かれていませんでした。 これでは、コードの見た目では大丈夫に見えても実際にはアクティビティログが送られてないケースが考えられます。

改善するには?

PRを作るときには、私が何をしたのか、どのような変更が加わったのかが一目で理解できるように心がけるようにしました。 例えば修正によって見た目に変化があるようなコンポーネントの場合には画面をキャプチャし、beforeとafterのgifを貼り付けるようにしました。これによってどのような更新をしたのかを、レビュワーは視覚的に理解することができます。

見た目に関して変更がない場合は、その変更が実際に動いているかのエビデンスを貼り付けます。例えばアクティビティログに関しては、ブラウザの開発者ツールのNetworkタブから、サーバに対して適切なアクティビティログが送信されているかを確認し、そのスクリーンショットを貼り付けるようにします。 これによってどのような変更がされたのかをレビュワーが理解できます。

改めてレビューを振り返ると、PRを作る際には

  • 見た目や動作の変更があった場合、beforeとafterをキャプチャして貼り付けておく
  • 見た目に変更がない場合には、その変更が正しいのかどうかのエビデンスをなるべく貼り付ける

を考えておくとよさそうです。

番外編 リーダブルコード

今回コードの可読性や命名について話しましたが、リーダブルコードという本にはより詳しい、様々なコーディングをするに当たってのtipsが書かれています。エンジニアとしてもっと成長したい方におすすめしたい一冊です（私も読まなければ）。

www.oreilly.co.jp

まとめ

レビューを大きく４つに分類し、個人的な改善方法について書きました。 振り返りで改善点を提示してはみましたが、いまだにPRを投げるとコメントが20件弱来るのでまだまだ学ぶことがいっぱいです。

この振り返りで得られたことを実践し、一発でLGTMをもらえるよう精進したいと思います。


buildersbox.corp-sansan.com

buildersbox.corp-sansan.com

buildersbox.corp-sansan.com

© Sansan, Inc.