もうコードレビューなんてしないなんて言わないよ絶対

こんにちは。ポケコロツインクライアントエンジニアの川村です。今回はエンジニアなら誰もが知っているであろう、コードレビューについてお話をさせて頂きたいと思います。

コードレビューとは

チームメンバーの書いたソースコードを読み、ツッコミを入れることでコードの品質アップを図る行為です。

コードレビューのメリット

  • 知力アップ:「この関数を使うと便利だよ!」というように、レビューをしたメンバーが知識をひけらかしてくれたり「へー、こんな書き方できるんだ!」というように、自分が知らない構文が見れたりするのでプログラミングの知識が増えます。
  • 保守性アップ:コーディング規約や設計思想が守られているか等、自分だけでなく人からもチェックをしてもらう方がより正確なコードに仕上がります。
  • 友好度アップ:チームメンバーが一丸となってコードの問題解決に挑むこともある為、協調性が育まれ、結果仲良くなります。
  • バグ件数ダウン:「あれ?ここバグってね?」というように、QAにバレる前に誰かにバグを見つけてもらえるかもしれないので、バグ修正及びQAコストの削減に繋がります。

コードレビューのデメリット

  • 時間がかかる:ちゃんとレビューしようとするとそれなりに時間がかかるのと指摘内容によってはメンバーと話し合いに発展することもある為、作業時間が圧迫されます。
  • 宗教戦争が始まる:「え、そんなんどっちでも良くない?」みたいな箇所を指摘してしまうとおっぱじまるやつです(気を付けないと面倒)

コードレビューはやるべき?

タイトルでもう言っちゃってますが、コードレビューは絶対にやるべきだと思っています。レビューをやることにより製品の品質が上がるのは勿論のこと、エンジニアとしてのスキルアップも期待できるので、時間を犠牲にしてでもやる価値のある行為だと思います。

コードレビューのコツ

ここからは快適なレビューライフを送る為のちょっとしたコツみたいなのを紹介させていただきます。

仕様を確認しておく

レビューをするコードに該当する箇所の仕様を把握しておくと、仕様漏れや認識違い等にも気づけてGOODです。

(まあ大変なのであまりできていないですが\(^o^)/)

レビューの観点を絞ってみる

バグはないか?仕様漏れや違いはないか?設計思想に沿った実装をしているか?最適化は行われているか?等、レビューの際に何を重視するかを1つに絞って確認をしてみると指摘箇所が見つかりやすくなると思います(初心者はスペルミスがないかや命名がおかしくないか?を見るだけでも勉強になるかもしれません)。

分からないところは質問してみる

コードを読んでみて「日本語でおk((何を言ってるのか分からないという意味))」と思った箇所に対しては素直に「これは何をやっているのでしょうか?」と質問するのが良いと思います。特に新卒の方等は「自分が先輩に指摘なんておこがましい・・・」という気持ちもあるかもしれないので、指摘ではなく質問をする形でレビューに貢献するのもアリです(質問された側は自分が書いたコードが分かりづらい事に気が付くし、説明をすることによってコードの理解度も上がる為)。

レビューの粒度は小さめにする

ちょっと極端ですが、レビューの依頼が来た際に↓のような大量の差分を見るとあまり見ないでLGTM((「Looks Good To Me」の略で、確認をしたコードが問題ないことを伝える時に使う))って言いたくなってしまうのでコードの量は抑えめにした方が良いです(1機能だけや500行以内等)。どうしても大きくなってしまう場合は「クソデカコードで済みません!」と謝ってからレビューをしてもらうようにしましょう。

レビューされる前にマージしてしまう

レビューを以下の流れでやっている場合、

レビュー依頼を出す→OKをもらう(または指摘され、修正後OKをもらう)→マージをする

レビュー依頼を出しても中々レビューされないことがあるので、先行してマージをするとLGTM待ちでモヤモヤすることがなくなります。また、レビューする側も自身の手を止めずに好きなタイミングで確認できて良いです。

ただしこちらの手法を取る場合、マージ先のブランチに気を遣う必要があります(リリース予定のブランチではなく開発中のブランチにマージをする)。

ここからは実際にコードを確認する時に役に立つ(かもしれない)バグ検知系の情報を紹介致します(今現在私がUnityエンジニアの為、完全にUnity + C#なお話になります)。

whileループに注目する

無限ループになるケースがないか確認しましょう。

例えば↓の場合、animatorが非アクティブになっているとループ内でUpdateを呼んでも永遠にanimatorのnormalizedTimeがインクリメントされないので、無限ループになります。

メモリリークがないかを確認する

UnityではリソースのLoadやオブジェクトのInstantiate、メッシュやマテリアルのコピー等でメモリリークを起こす可能性があるので解放処理を書いているかチェックしましょう。

また、UniRxを使用している場合もSubscribeで登録したObserverを解除する必要があります。

↓とか

↓の後にdisposablesをDisposeしていないケース等

アクセス違反警察になる

配列の範囲外アクセスやDestroy済みオブジェクトの操作等、ランタイム時にエラーになるケースがないか確認をしましょう。

以下のようなメソッドがある場合、もしも終了処理(OnDestroy等)以外の所でオブジェクトをnullにした後や、オブジェクトを初期化する前にそのオブジェクトにアクセスしている箇所を見つけたら「異議あり!」できます。

非同期処理は要注意

これとこの非同期処理は並行して走らせても良いんじゃない?」みたいな平和なやつなら良いのですが、全く違う場所でやっている非同期処理の同期待ちをしないといけなかったみたいなこともあるので、StartCoroutineasync/await等の単語が出てきたら深掘りしてみると良さそうです。

例えば以下のようにボタンクリック時に外からセットしたメソッドを呼び出そうとした場合、

セット元が以下のようなコードになっていたらonClickの非同期処理が終わってからgetSpriteを呼ぶ必要があるのですが、セット先では同期待ちを行っていない為、意図した結果が得られません。このように別クラスになっていて見つけ辛くなっていることも良くあります(そもそもasync voidは良くないのでそこで指摘がきそうではある)

最後に

コードレビューは真面目にやると割と時間がかかるので忙しい時はスルーしたり適当にやってしまいがちですが、きちんとやるととても勉強になるので出社直後等「レビューだけをする時間」を設けてみるのをお勧めします。

ココネでは一緒に働く仲間を募集中です。

ご興味のある方は、ぜひこちらのエンジニア採用サイトをご覧ください。

→ココネ株式会社エンジニアの求人一覧

Category

Tag

%d人のブロガーが「いいね」をつけました。