指摘されたプルリクで打線組んだ

皆さんこんにちは

髪の毛セルフカットエンジニアの金子です。

弊社では開発時にはGithubのPullRequest機能を使ってコードレビューを行っています。
コードレビューをやるからにはそれなりの時間が必要でコストも掛かりますが、必要なことなので都度状況に応じて調整しながらやっています。

レビュワーも人間なので、チェックの厳しさは人それぞれですが、私は影響範囲が大きいような部分や重要度の高い処理、時間的に余裕があるときには厳し目に見てもらうように心がけています。
厳しく見てもらうと、仕様を満たす以外のところでもこだわるのでより時間がかかりますが、コードベースをきれいな状態に保つことで開発のパフォーマンスを下げないようにできると思います。
単発で作って終わりというものはほとんどないので、仕様を満たしていたとしても手抜きをすると後で大変な思いをすることになります。

弊社の提供しているEAPではアプリ計測が標準でついてくる上に、アプリをリリースしたあとにデータを解析して次の施策につなげるようにご提案させていただきながらアプリを発展させていくということが多いので、改修を重ねていってもきれいな状態を保つということは実装時間の短縮につながると思うのでお客様にとっても良いことだと考えています。
かけなくていいことにできるだけ時間をかけずに、本当に必要なことに時間をさけるようにしたいと思っています。

話はだいぶコードレビューからそれてしまいましたが、指摘されたプルリクを一部紹介したいと思います。
※今回はレベルの低いようなものをあつめていますが、実際はもっとレベルの高いやり取りをしています。

打線

1　一　typoではないでしょうか
2　遊　メソッド名がおかしいです
3　二　テストを書いてください
4　中　ビルドできません
5　右　Issueに関係のない変更を含めないでください
6　三　printが残っています
7　左　インデントがずれています
8　捕　コメントが間違っています
9　投　この処理は必要あるのでしょうか

解説

1　一　typoではないでしょうか
たまにあります
たしかにtypoなので黙って修正です。

2　遊　メソッド名がおかしいです
あとから考えてみるとなんでこんな名前にしたのかわからないというときがあります。
迷ったら他人に相談するようにすれば解決するかもしれません。

3　二　テストを書いてください
すべてのテストをかけないのでテストを書く範囲は決まっています。
なぜか書き忘れることがあります。

4　中　ビルドできません
私はいったい何をやっているのでしょうか
「37年間必死に生きてきた結果がこれなのか？」という気分になります。
恥ずかしすぎるので今はセルフレビューは3回するようにしています。
7回セルフレビューすることもあります。
セルフレビューのおかげでこのような指摘をされることはなくなりました。

5　右　Issueに関係のない変更を含めないでください
バックログなりで課題を立ててその課題に対応したプルリクを出すというのがいつもの流れですが、
たまに悪魔の誘惑に負けて関係のない変更を含めてしまうことがあります。
レビュワーにとっても負担なので、できる限りこのようなことは無いようにしたいと思います。

6　三　printが残っています
デバッグコードが残っていたシリーズです。
海よりも深く反省しながらコードを消します。

7　左　インデントがずれています
これはどうでも良いシリーズですが、多分おおくの人にとってはどうでも良くないので申し訳ない気持ちでいっぱいです。

8　捕　コメントが間違っています
コメントはできる限り書くようにしていますが、たまにコピペしてそのままになっていることがあります。
これもセルフレビューをすることで回避できると思います。

9　投　この処理は必要あるのでしょうか。
不要です。

以上、指摘されたプルリクの紹介でした。
レビューはできる限り確認をした状態でレビュワーに回さないと見る方の負担が大きくなってしまうので、セルフレビューもそうですがSwiftLintなど機械的にできるものは導入しています。
今後も改善してストレスのないレビューを心がけたいと思います。