自分がコードレビューで気をつけていること（2020.5 iOSアプリ開発版 ）

投稿日: by yimajo

わたくし@yimajoが仕事のコードレビューで気をつけていることを書いておきます。特にiOSアプリ開発をしているので内容はiOSアプリ開発におけるコーディングやStoryboardについて書いていますが、その前提やメンタル面の話が多めです。

ちなみにiOSアプリのコードレビューで見ているポイント 2020年5月版にインスパイアされて書いてみました。

前提

  • レビュー時にリスクや課題、問題という言葉を意識して使い分ける
    • 「このコードは問題です」という指摘をするとき、「問題」という言葉の意味をなるべく共通の理解を持つ
      • 問題
        • 現在起こっている正常でない状況
          • 例: 仕様を表現できていない
          • 例: 不具合がある
      • 課題
        • 問題を整理/分割したもの
          • 例: deprecatedなAPIを使っている
      • リスク
        • 時間の経過もしくは何かの要因によって課題や問題になってしまう事柄
          • 例: テストコードが書けていないので手を加えると壊れる
          • 例: Swift言語でAppleが解説するガイドラインと違うやり方をしていて読みづらくさせる
          • 例: アルゴリズムの実装に無駄がある
  • コーディングは読みやすくすることも考えていく作業
    • 「何をやりたいかという仕様」を読み取りやすくすることが他者を助けることになる
      • 作ることだけを目標としている人には「読み取りやすさ」も大事だと説明する
      • 「読み取りやすさ」の感覚について理解できない人もいる
        • コーディングの「ガイドライン」「ルール」をSwiftLint（後述）などでシステム的に導入する
  • ガイドラインが良くないと言われたらそれに対して柔軟に対応すること
    • 「ルールだから」と思考停止しない
      • 思考停止してはいけません、とストレートに話すのは避ける
        • 説明するのは最も難しい
    • 雑談しつつコードの話や設計について話すことと良かったりもする
      • 雑談のゆるい段階（相手にタスクがない状態で）ちゃんと熱意を持って真剣に話す
      • SlackなどでURLを貼るのはおすすめしない
        • それは「読め」という押し付けになりかねない

Pull Requestによるコミュニケーションを何度かしていて、「同じような指摘を何度もしてしまってるな」とか「レビューのコミュニケーションが良くない方向に向かうな」となる際にもこの前提というやつが揃ってないことを疑うと良いと思います。このような前提は自分たちの方向性によって決めるのが得策でしょう。

iOSでコードレビューする場合の資料

基本的にはAppleの資料をベースとするのが誰にとっても良いでしょう。

細かなメンタル的な話

  • 間違いだと指摘するときほど気をつける
    • 相手はこちらが指摘しようとしていることを承知の上でやっていることもある
    • 「注意してやろう」みたいなのもよくない
      • 相手を詰めない
  • IMO, NITSなどハイコンテキストなことを避ける
    • 例えば IMO とか使われて初見でググらずにわかった？
      • 「自分の意見だけど」って書けば相手がググらなくていいだけじゃない
    • NITS なんて「これは些細なことなんですが」、でいいんじゃない？
      • でも些細だったら指摘しなくてもいい
  • 本当にどうでもいいことなら指摘しないで自分でやればいい
    • 「これは無視してもらってもいいんですが」と書くなら、その時間を別のことに使ったほうがいい
      • 仕事で無視なんてできるわけがないので、そのために時間を割くことになる
    • どうでもいいことや好みであると最初にわかってるならレビューではなく、雑談として別で話す方法だってある
  • どうしても修正してほしい時、つまり課題や問題に直面するようなコードになっている場合
    • 課題や問題に直面しているので修正してほしいとお願いする時
    • 自分がすぐやれることや、自分のほうがうまくやれることなら
      • Suggest Changeの機能を使う
        • Suggest Changeでコンパイル通らないときもあるけど、それはCIとかですぐ検知できるから課題にはならないし、笑って済むよ
      • Pull Requestのブランチに対してPull Requestを出す
        • もうほんとこうしてよねってときはPRにPRを出す
          • 同じ取り組みに対してレビュアーとレビュイーの立場が逆転する
  • もしリスク/課題/問題の見極めが難しいならドワイト・D・アイゼンハワー大統領の「重要度と緊急度のマトリックス」を思い出す
    • 緊急でもないが重要であればそれは課題かもしれない
      • そうなるとPull Requestをブロックするほどでもない
        • issueを作成し、レビューコメントでissueを作成したことを伝える
  • 伝え方
    • そのコードに「違和感があります」「わかりづらいので」とかは具体性を欠いていると認識したほうがいい
      • 「違和感がある」というのは言いたいことはわかるが、それは経験からくる直感と異なるという話
        • 具体的に説明をするには「前のプロジェクトではこうしていたが、今回とは違う」とか
          • そうだったら「今回のプロジェクトでは設定が違う」など答えが明確になる
          • 「違和感がある」と言われても言われた側はその違和感の答えにたどり着くのは難しい
          • そもそもOSSのコードを良く読む人と読まない人で圧倒的に感覚は違う
    • 指摘は事細かく、ブレの無いよう厳密に行う
    • 「インターネットにも書いてあります」とは言わない
      • 世の中には様々な意見があるので、どっちの意見もある。それは自分のプロジェクトにも適用できるのかよく考える
    • もしマウンティングしてくる相手に対してこちらがマウンティングしそうになったら自分の知らない分野のことを想像する
      • （´-｀）.｡oO
  • 話がこじれたら対面で話そう
    • 話がこじれることはある
    • Slackとかの文字コミュニケーションは補助的なものでしか無い
    • どこかで対面コミュニケーションするか見極める
      • Pull Requestのレビューで＜1時間かけて丁寧に沢山のコメントを打つ＞のと＜対面（Web通話）で30分＞なら後者のほうが早い
        • 1対1でなくたっていい、リーダー的な存在の人を巻き込んで話せばいい

仕組み的な話

基本的にルールを作っても大抵の人は守れません。ドキュメントにコーディングガイドがあってもそれをチェックするのは面倒です。そのために仕組み的にそのルールに従えるようなガイド的なものを用意するのが良いでしょう。「このルールに従ってねーずら」という指摘は本当に人間がやるべきじゃないことで、自動化出来ます。

コードへの指摘について

ここからやっと本題です。

再代入しない変数はvarではなくletにしましょう

varを使うと変数が変更されるために使われます。つまりコードを読んでる人は、「いつ」「なんのために」「どこで」変数が代入されるんだろうと思うわけです。letを使えばそのようなことを考えることがありません。言語仕様的にできないから考える必要を排除することができるのです。

変数は基本的にはまずletで宣言し、変更する必要が出たらvarにしたら良いでしょう。

しかし、「ローカル変数なんてvar使って再代入しなければXcodeで警告出るのでは？」と思ったあなた、それは多分そうです。しかしプロパティのvarは警告されません。

guardを使ってメインの処理とそうでない処理を分けましょう

関数やメソッドの処理を読む際に、そこで本当にやりたい処理の分岐と、チェックのための分岐が別れていれば分かりやすいでしょう。

そのためにもguardを使っていれば分かりやすいことがあるはずです。

余談

自分たちのチームでguardが扱いきれない/余分な議論が発生すると判断したら使わないのも手だと思いますが、そうではなく使い方のコンセンサスを持つということを試みてほしいということを書いておきます。

guardは条件がfalseの場合にelseが書かれているガードステートメントを実行します。
そのためifとは 逆 の印象を受けます。

guard condition else {
    statements
}

しかし 逆 という考え方は厄介さを生むので、この先の処理を実行しない条件を記述する と考えるほうが混乱しないですみます。
もし逆という考え方でguardを使うと「guardはややこしいから使わないようにしよう」と思ってしまうため、もちろんそういうガイドラインも世の中には存在します。

それはそれでguardに慣れている人からしたら面倒なルールになります。そういう記事別途書いておきました。

weakかunownedのどちらを使うべきか

unownedで書かれたコードをレビューする際に、本当にunownedのほうが妥当かということを調べるきっかけになるので、unownedで書くことは良いとは思います。
でも、本当にunowedで良いのかを調べることは結構コストがかかるので、アプリのどうでもいい場面では「weakにしても良いよね」、というコミュニケーションをします。

具体例を書くと、例えばユーザのお金に関わるアプリを作っていたとして、そのコアのバリューのために様々な外部SDKを使っていたとします。
そのSDKを使って非同期処理のクロージャでunownedを利用しているような場合は、それが正しいかどうかを厳密に調べれば調べるほど品質は上がるはずです。

一方、アプリのコアなバリューではない実験的な機能を早期に導入する際、つまり「品質よりスピードを優先する」という場面ではweakで十分だと判断できるはずです。
そしてその機能を改善していく際にunownedにすることもあるでしょう。

  • レビュアーはunownedで書かれているコードについて本当にunownedでよいかレビューする
    • しかしその時間がない、unownedで良いか判断できないときはそれを伝え、なぜunownedにしているかの根拠を聞く
    • 根拠がないなら「weakでいいんじゃない」というコミュニケーションをする

余談

クロージャでweakやunoowedを使わなくてもメモリリークしない場合もあります。

SwiftUIのViewはなぜクロージャでselfを書いても循環参照しないのか
https://qiita.com/yimajo/items/4dde5b297f61b699d844

他にはUIViewのアニメーションなど、クロージャが実行されている間はもちろんクロージャが保持されますが、即時アニメーションが終了すればクロージャは破棄されます。そのためにクロージャが保持しているselfも破棄されることで循環参照しない場合もあります。そういう場合、weakかunownedかはどちらでもいいし無くてもいいです。

  • クラスでなければ参照されない
    • 参照カウントがそもそも上がらない
  • UIView.animate()などによるクロージャは即時実行される
    • 参照カウントは上がるがすぐクロージャ実行され下がるのでどうでもいい

TODOをコードに書いてpushしない

OSSならTODOとかFIXMEと書いておくのは有効だとは思いますが、仕事で // TODO: とコードに書いて"リモートリポジトリ"にpushしてしまうと、それいつまでに誰がやるべきかそれには何が必要か判断しなきゃいけなくなりませんか？

誰にボールがあるのかわからないので自分は避けてほしいと思います。pushするまでは問題ないですが、pushする前に削除してほしい。そして、情報共有する必要があるならissueなどを作成してくれるほうがいいです。

テストコードの優先度

テストコードは可読性の優先度が実行効率より高くしたくなるはずですから、開発初期は「可読性 > 実行効率」でレビューしていいと思います。具体的な指摘としては「実行効率は落ちるけど、可読性が高い別の方法もありますよ」って感じです。

リリース後に運用を続けると自然とテスト時間が大きな課題になるでしょう。
Pushしたりマージした結果をテストして、
それを都度社内で配信するなどのサイクルが遅くなると開発効率が悪くなりますし、最悪テストがおそすぎてCIがタイムオーバーになったりします。

そうなると無駄に実行時間が長いコードや重複を取り除かなければいけなくなるので、その際に効率を気にするレビューをします。
具体的には「実行効率の良い別の方法もありますよ」という感じです。

テストコードがテストパスさせるためだけのコードになっていないか

無理やりテストをパスさせるようなコードに意味はなく、そんなテストを書かないようにしましょう。

テストコードがたまたまパスしていないか/ライブラリの内容をちゃんと把握しているか

ライブラリの使い方が正しくなく、テストコードがたまたまパスしているようなケースでもその原因に至る経路を示します

  • 悪い例
    • 「ネット上の浅いブログを浅く理解してコードを書いているようだけど、Quickのコードを読めやー」
  • 良い例
    • 「正しくQuickが使えていないようなので、一緒にライブラリの内部で何をやっているか10分くらい追いましょうか」

StoryboardのAutoLayoutについて

AutoLayoutなど見た目に関わる部分は適当にやってもなんかそれっぽく動く。しかし、自分の思ったとおりにならないときは徹底的にならないわけです。そのように自分のミスが分かりづらいため、初学者は 「何が分からないかが分からない」 状態に陥いりやすいし、時にはそれっぽくやって乗り切ってしまって理解しきれていない場合があります（gitとかもそうかもしれない）。

これについては原理原則を知る必要がある。しかし対面もしくは画面共有（もしくはスクショ）で下記について説明すればいいはずです

  • 考え方の重要な部分さえ説明すれば思い込みや手癖を避けられる
  • わかった気でいる人でも本当にわかってはないということを明らかにする

privateであるべきものがprivateになっていない

プロパティやメソッドをprivateにし忘れるのはよくあることです。

さくっとSuggest Change使ってprivateって打つだけで早いと思います。Pull Request出した側が確認せずSuggest Changeを受け入れてしまってコンパイルエラーになったとしてもCIが動いていればわかります。

「yimajo、おめーのSuggestを受け入れたらコンパイルできなかったじゃねーかｗ」ということはありますが、こちらが指摘だけでなく自分でコードを書き換える人間だと理解して貰えれば、それでいいのではないかと思います。

finalになってないのはどうでもいいかな

自作のクラスをfinalにするかどうかは些細なことなのでほぼ指摘しません。

  • モチベーション
    • finalをつけようがつけまいが自分は継承を良しとしないのでどうでもいい（他人がfinal付けるかどうかは好きにして）
      • 継承で問題解決せず、他の方法を提案する
  • 会話
    • 「finalつけないと継承していいと思うんじゃない？」
      • 「finalがついてようと継承したいやつはfinalを外してくる。よってSDKではない自分たちのコードにfianlがついてることでそれを禁止していると表現できない」

! force-unwrap を使うかfatalError()を使うか

また今度書きます...

雑談

コンフリクトした際のマージについて

例えばGitHubでfeatureブランチをdevelopブランチにPull Requestを出してコンフリクトすると表示された際。まずfeatureへdevelopをマージすると思う。そうしてGitHub上にpushするとmerge developのように表示されるが、ローカルのdevelopではなくリモートのdevelopをマージするほうが確実じゃないかな、という雑談をします。そうするとmerge origin/developのように表示されるはず。

$ git merge origin/develop

これは必要ないかもしれないので、レビューではなく雑談として

おわりに

『ヴァイオレット・エヴァーガーデン』というアニメ化された物語があるんですが、ラブレターを仲介して書く仕事をする主人公の話なんですよね。この主人公のヴァイオレットというのが戦場で拾われて、戦争帰りのそういう環境に特化させられた少女兵なわけでめちゃくちゃ強い殺戮少女なんです。そして当然人の心というものがわからないし自分の気持を伝えることも苦手なわけです。だから最初は依頼人の気持ちは理解できず、ラブレターを仲介する仕事なんてうまくいかったりするところを、様々な仕事を通じて成長していくという物語です。

そして我々の仕事におけるコミュニケーションでも、このヴァイオレットと同じように少女兵的なスタイルをとってしまっていないでしょうか？とね、そういうことを思ったりしています。