inFablic | Fablic, inc. Developer's Blog.

フリマアプリ ラクマを運営する Fablic の公式開発者ブログです。Fablic のデザイナー・エンジニア・ディレクターが情報発信していきます。

FRIL iOSのコードレビューで気をつけていること

こんにちは、Fablicのモバイルアプリエンジニア@huinです。

宗教上の理由で長年Apple純正キーボードを使い続けてきましたが、 肩こりがひどくて最近ErgoDox EZをポチりました。 クリスマス頃に届く予定なのでガラにもなくクリスマスを楽しみにしているところです。

Fablic Advent Calendar 10日目の記事ということで、 今日はFRIL iOSのコードレビューで意識していることについてお話したいと思います。

FRIL iOSの開発環境

本題に入る前に、FRIL iOSの開発環境について軽く説明をしておきます。

弊社では、GitHubを利用したPull Request(PR)とコードレビューによって日々の開発を行っています。 FRIL iOSにコミットしているエンジニアは現在3名で、その中でお互いにコードレビューをしながら開発しています。このあたりは一般的な普通の開発スタイルだと思います。 (ちなみに3名は十分な人数ではないため現在iOSエンジニアを積極採用中です。ぜひ弊社採用ページをご覧ください)

こういった開発環境で円滑にコードレビューおよびQAを行うために、 PRを作成する時には以下の4点を記述することを求めています。 (どうしてこれらの情報が重要かは弊社黒川のレビューしてもらいやすいPRの書き方を読むことでご理解いただけると思います)

  • 対応するIssue
  • 具体的な変更内容
  • 動作の確認手順
  • スクリーンショット(UIの変更を伴う場合)

実際に僕が作成したPRのスクリーンショットが以下になります。

f:id:infablic:20171210164412p:plain

このように、変更の目的やそれを確認するための手順をPRに明示することで、 レビューがしやすくなるような工夫をしています。

なぜコードレビューを行うのか?

ところで、そもそもなぜコードレビューを行うのでしょうか?

人によっていくらか違う意見があるかもしれませんが、 僕自身は、コードレビューによって以下のようなメリットがあると考えています。

  • 実装者が見落としていた仕様や実装の漏れに気づくことができる
  • レビューによって変更の意図や仕様をレビュアーが理解し、コードの属人化を防ぐことができる
  • 実装者、レビュアー間でドメイン知識や技術力の共有が行われる

コードレビューはそれなりに時間のかかる作業なので、 開発スピードを意識してレビューを行っていない開発現場もあるかもしれません。 しかし、複数人での開発で中長期的にプロダクトの品質を維持するためには、 時間を犠牲にしてでもコードレビューを行うべきだと考えています。

コードレビューで気をつけていること

さて、いよいよ本題です。

先にあげたコードレビューのメリットから、 僕は良いPRと良いコードレビューがそろって初めて良い開発ができると考えています。 メンバーが良いPRを出してきても、ずさんなレビューを行っていてはアプリの品質が上がることはありません。 レビューによって得られるメリットを最大化するためにも、 良いレビューを心がける必要があるのです。

というわけで良いレビューを行うために意識していることを5つ紹介します。

1. 仕様に問題はないか?

PRあるいは対応するIssueには、その変更で達成すべき内容(つまり仕様)が書かれているはずです。 iOSアプリであれば、多くの場合デザイナーのUIデザインや サーバーのAPI仕様に沿うように実装がおこなわれていると思います。

僕はレビュー時にまずその仕様が本当に十分なものかを確認します。

たとえばUIの変更を含んでいる場合には、 各種iPhoneの画面サイズやiPadでも無理なく表示できるUIになっているか? といったことを考えます。 また、サーバーと通信する場合には、 APIの仕様は十分にシンプルなものになっているか? エラー処理まで含めて設計されているか? Android版と仕様のズレがおきていないか? ということを確認します。

このように、コードそのものを見るまえに、 そもそも仕様 = 設定されたゴール自体が間違っていないか? を入念に確認します。

2. 仕様に対して適切な設計・実装ができているか?

仕様に問題がないことを確認できたら、 次はその仕様に対して適切な設計・実装ができているか、 実際のコードを確認していきます。

仕様に対して常に正しい実装ができていればいいですが、 言語の仕様や既存の実装によっては常にそれが実現できるとはかぎりません。 また、実装中に試行錯誤をしていると、 仕様上実装はできているけど他人から見て読みにくいコードになってしまうこともあります。 仕様を満たすコードになっているか?他のメンバーが理解しやすい実装になっているか? といった点を意識してコードを読んでいきます。

3. 変更によって予期しない箇所に影響を与えていないか?

これは1つ目の仕様が適切か?という部分と重複する部分ですが、 再確認の意味も含めてコードを見たあとにあらためて確認するようにしています。 意識している点は以下の3つです。

  1. コードレベルで、実装によって周辺の実装に影響を与えていないか
  2. サービスの仕様として他の画面・機能に影響を与えていないか
  3. iOSアプリケーションとして問題を起こしていないか(プッシュ通知などのAPIを利用している場合)

これらはGitHub上で変更点だけを見ていても気づけない部分です。 また、変更による影響範囲が完全に検証できていれば不具合は発生しないはずなので、 この確認は常に不完全に終わっている部分でもあります。 そういう意味でも非常に難易度の高い作業ですが、 コードをチェックアウトして周辺のコードを見返すなどして できるだけ精度高くできるように意識しています。

4. 確認手順に漏れはないか?

前述の通り、FRIL iOSのPRには加えた変更の確認手順を含めるようにしています。 本来は、すべての変更についてユニットテストやUIテストを書ければいいのですが、 現状の開発環境・リソースではそこまで実現できていません。 そのため、最低限開発者が正しさを確認する手順をもれなく記述できることを求めています。

そして、テストコードの代わりとして確認手順が正しいかどうかをレビューします。

例えばFRIL iOSの場合、 ユーザーはメールアドレスとパスワード以外にもSNSのアカウントを利用して登録・ログインすることができます。 ログイン画面やAPIに変更があった場合にはメールアドレスでのログインだけでなく、 SNSアカウントを利用したログインにも影響がないかを確認する必要があります。 開発者自身がテストケースを網羅できるのが理想ですが、 チームとして網羅できるようにレビューときに入念に確認します。

5. 本当に正しく動くか?

頭の中で組み上げたプログラムが正しく動いてくれればいいのですが、 えてして想像通りに動いてくれないのがプログラムです。 仕様、実装、テストケースを入念にチェックした上で、 最後はやはり実際に動作させてみて仕様どおりに動いていることを確認します。

特にUIの変更が含まれている場合には、 ではサポートするデバイス/OSバージョンすべて確認するように意識しています。 FRIL iOSではUIの変更をともなうPRにはスクリーンショットを添付するように求めていますが、 アニメーションなども含めたUI全体の操作体験は、 実際に動かして確認する方が良いと個人的に考えています。

時間はかかりますが、QAに入ってから手戻りすると精神的にもコストが高いので、 PRレビューの時点でなるべく網羅的に確認するように心がけています。

まとめ

FRIL iOSのコードレビューで気をつけていることを5つ、紹介させてもらいました。

たかがPRのレビューにコストをかけすぎていると思われたでしょうか? 実際のところ、僕自身もPRのレビューは非常に疲れる作業です。 しかし、プロダクトの品質を維持していくために、 問題を含んだコードをプロダクトに入り込まないようにする努力が必要です。 専任のQAチームや自動テスト環境があればなとは思いますが それがないからといって品質を下げていい理由にはなりません。 また実装者と同じレベルでレビュアーが変更について理解することで、 コードの属人化を防ぐこともできます。 そういう意味でも、良いレビューを意識することは重要です。

以上、FRIL iOSのコードレビューで気をつけていることの紹介でした。 もし、上記以外にもこういう点に気をつけてレビューしてるよ!というポイントがあれば、 ぜひTwitterなどでコメントしてください。