概要
レビューってなにするの？品質のために必須？
でも、レビューしなくてもうまく行っている現場あるよ？
・・・
自分は、レビューする現場としない現場のその両方を経験しているので、
その現場の違いから、レビューの目的・心がけなどを紹介したいと思います。
なにか違うってところあれば、コメントいただけると助かります。
紹介すること
- 現場によって求められるレビューが違う
- レビューの目的
- 目的とのトレードオフがある
- レビューのやり方
- レビューワーとレビューイの心がけ
- プルリクについて
- チームとしての活動
ふれないこと
- 良いコーディングの方法
- テスト、自動テスト
- ペアプロ
- アジャイル
- 品質とは
1. 現場によって求められるレビューが違う
過去はSES業務、今は事業系を経験
- SESとは、委託 契約 で、システム開発することです
- 事業系は、自分たちでWebサービスを提供する企業のことです
ウォーターフォールな現場の場合
- ウォーターフォールは、契約書を中心とする開発手法
- ウォーターフォールは、契約書ベースなSESやSIerに多い ← 過去はこれ
- 契約書ベースなので、要求の変化が少ない。不具合でなければOKな世界 (逆に、契約内容がコロコロ変わったら問題)
- 言い換えれば、品質よりも契約に沿っているがどうかが大事で、レビュー以外に工数をかける
- 更に言い換えると、契約に沿っていれば、最悪、お客様に謝まればOK。。(何かあっても、契約通りであれば、お客様は強く言えない)
アジャイルな現場の場合
- アジャイルは、ユーザーを中心とする開発手法
- アジャイルは、ユーザーベースな事業系企業に多い ← 今はこれ
- ユーザーベースなので、要求が常に変化する。変化に強いコーディングを要求される世界
- 言い換えれば、品質よりも事業が優先なので、事業の成長を妨げないコードにすることが重要
- 更に言い換えると、ユーザーに謝ってもユーザーは帰ってこない。。(帰ってこなければいけない契約なんて無い)
結論
- アジャイル開発なら、レビューしたほうが良い
- 要求が絶えず変化する現場は、レビューしたほうが良い
- 契約がない現場ほどレビューしたほうが良い
2. レビューの目的 (ここから今の話)
改めてレビューの目的
- 事業の成長の妨げにならないようにする → 変化に強いコードにする
- 事業の信頼を下げないようにする → 品質の担保
- 事業の生産に時間を費やすようにする → 運用コストの削減
2.1. 変化に強いコードにする
- 仕様変更があっても工数がかからないようなコーディングにする
[もし、変化に弱かったら]
- この仕様変更のときは、ここと、ここと、あと、、どこを変更すれば良いんだっけ？？
- あっ、、うまく行ったと思ったのに、考慮漏れしてて本番環境で致命的不具合出た、、
2.2. 品質の担保
- 他人の目線を取り入れることで不具合を未然に防ぐ
[品質担保の心構え]
- 致命的なバグでユーザーが逃げないように頑張ってレビューする
- 品質の担保は、レビューよりもテストが重要である。また、テストの自動化も大事
2.3. 運用コストの削減
- 保守性の高いコードにして、機会損失を防ぐ
- 可読性の高いコードで、不具合にすぐ対処できるようにする
- 可読性の高いコードで、メンバー受け入れを簡単にする
[もし、保守性が低かったら]
- 運用に多量の時間を割いてしまって、事業の成長に費やす時間を奪う
- どこのファイルのどこの行を修正すれば良いのかわからなくなって、手当たりしだいにコードを見て、時間を浪費する
[もし、可読性が低かったら]
- １ファイルに長いコードが絡み合って、どこに何があるかわからないようなスパゲッティコードが生まれている
- 実は、ジョインしたばかりの新人が、分かりづらいコードだなぁ。。と心のなかでは思っている
- 「このコーディングわかりづらっ」って思わず口ずさんだら、隣りにいる先輩のコードだった。。
3. 目的とのトレードオフがある
トレードオフの内容
- 時間(コスト)がかかる
- 理由
- レビューの人数は、多ければ多いほど品質は上がるが、単純計算で工数も積上がる
- 心構え
- 品質よりも事業の成長の方が大事だったりするので、品質にとらわれ過ぎないようにする
- レビューは工数とのトレードオフなので、本当は何が重要なのかをよく考える
- 具体例
- 例えば、小さな不具合であれば、修正よりも、事業のためになる機能追加を優先したりする
- 例えば、小さな不具合であれば、しばらく放置して、次回のリリースで修正するとう選択肢もある
- 理由
目的以外のメリット
- メンバーのスキル把握
- 理由
- スキルの把握ができれば、仕事を振りやすい
- 理由
- メンバーのスキル向上
- 理由
- 他者のコーディング技術・知識・考え方を、レビューを通して吸収できる
- 理由
- 属人化の排除
- 理由
- レビューをすれば、自然と仕様・設計・実装の共有ができる
- 理由
- 一人に実装を委ねるのは、リスクしかない (その人が休んだ時に誰も対応できないなど)
- 理由
4. レビューのやり方
レビューの種類
- 仕様のレビュー
- 設計のレビュー
- コードのレビュー
4.1. 仕様のレビュー
- 基本的に、プルリク内で仕様のレビューはしない
- 実装前にレビュー時間を作ってするべき
- コーディングの異変に気づき、そこから、そもそも仕様おかしいくない？って時はある
- だけども、レビューでの仕様についての指摘は、言葉遣いに気を配る
- そもそも一生懸命に仕様を決めた上で、設計・実装へ進んでいるので、言葉遣いには注意する
- 自分もその仕様を決めた一員であると自覚し、他人事のような指摘の仕方はしてはならない
- ただし、指摘はするべきである
- できれば、プルリク内コメントですませず、対面でコミュニケーションしたり、slackで連絡を取ったり、などで工夫する
4.2. 設計のレビュー
- 基本的に、プルリク内で設計のレビューはしない
- 実装前にレビュー時間を作ってするべき
- 設計の共有は事前に済ませて、メンバーの同意をとっておくのがベター
- だけど、毎回、設計の事前共有を求めると開発のスピード感がなくなるので、何でもかんでも共有はしない。ほぼ設計=実装であるような小さい設計を共有されても、工数を浪費します
- 例えば、テーブルを追加・変更がある場合は、実装への影響反映が大きいので、設計を共有したほうが良いです
- が、やっぱりプルリクを見たタイミングで、設計が気になるって時はある
- その時は、コメントするべきである
4.3. コードレビュー
- プルリクが来た時のメイン業務です
- タイピングミスはないか
- コーディングレギュレーションに沿っているか
- コーディングレギュレーションは事前に決めておく
- O'Reillyのリーダブルコードは読んでおく (コーディングの教科書のようなもの)
- アーキテクチャに沿っているか
- 事前に知識の共有をしておく
- コードレビューはコードテストではない
- 基本的に、テストは実装者の仕事です
- ただ、怪しいコードがあったらローカルに落としてテストしてみたり、仕様を把握するためにテストしてみるなどは良いと思います
5. レビューワーとレビューイの心がけ
レビューワーとレビューイとは
- レビュワーは、レビューする人の事
- レビューイは、実装者の事
レビュワーの心がけ
- 進んでレビューする気持ち
- 実装者とレビューワーは相互関係にあります
- レビューを嫌々やっていると、実装側が気を使って、モチベーションが下がります
- 開発を促進させるためにも喜んでレビューを受けましょう
- 例：工数余っているけど、この機能開発は、次回へ見送ろうかな。。
- レビューのチェックリストを用意する
- Googleからコードレビューガイドラインが公開されています。それをもとに個人やチームでチェックリストを作るのも良い
- 修正コメントするときは、理由を説明する
- レビューは日本語で"批評"の意味で、"批判"ではありません。「ここができていない」「ここは○○にして」だけのコメントは、レビューになっていないのでやめましょう
- 修正の理由がないと、実装者はレビュワーの言いなりになってしまうので、やめましょう
- それに、エンジニアとして、修正の理由は説明できるるようになりましょう
- 否定よりも疑問、疑問よりも質問
- その人なりに考えてコーディングしています
- 自分が見えていないだけで、なにか意図があるかもしれません。理解できないことがあったら、悪いと決めつけるのではなく、質問しましょう
- スキル向上の心をもつ
- 他人のコードの良いところを吸収し、自分のスキルに取り込みましょう
- また、言葉もスキルの一つです。一生懸命レビューして怒られても、スキル不足なだけで、これから共に向上させましょう
- レビューは期限内にすませる
- レビューを後回しにして、ギリギリになって修正が発覚すると、スケジュールが詰まります
- レビューは当日、難しくても次の日までを心がけましょう
- レビューへのコメントは、細かく五月雨にしない
- 修正箇所が複数あった場合、一括で修正した方が効率が良い。細切れに修正させると、余計なコストを使わせることになります
- 相手は労働時間でお金を頂いている立場なので、余計な時間を使わせないことを意識しましょう
レビューイの心がけ
- 良いプルリクを出す
- プルリクを見る側よりもプルリクを出す側の方が大事です
- レビュワーが理解しやすいようなプルリクを出しましょう
- 良いコーディングする
- 略。これについてはふれない
- 技術力の向上
- 自分の技術力が上がれば、プルリクを見てくれているレビューワーの技術力の向上にもつながる
- レビューの負担を下げるコーディングをする
- タイピングミスを未然に防ぐために、テキストエディタに、自動でコーディング規則を守れるようなプラグインを入れましょう
- 他人が理解できるようなコーディングをしましょう
- 例: 関数名は無駄に略さない (文字が長過ぎるデメリットよりも、文字が理解できないデメリットのほうが大きい)
- 例: イレギュラーな実装をした場合は(例えば、あえて非効率な実装をした場合は)、コメントを残す
- プルリクのコメントに感謝を
- 感謝の気持ちを忘れない
- 感謝はしすぎても損はない
6. プルリクについて
プルリクに書く内容
- チームでテンプレートがあるはずなので、それを使う
- 以下はあったほうが良い。以下以外は、コード見れば分かる
- 概要・課題 (なぜこのプルリクを出すのか。外部リンクでも良い)
- 仕様
- 仕様に沿ったテスト一覧
- そのテスト結果
良いプルリクを出すコツ
- 変更が大きいプルリクは作らない
- 大きいと、可読性が低いので、不具合を見逃す可能性が上がる
- 大きいと、どこのコードを変えたか把握するのに時間がかかり、レビューコストが増大する
- 読み安い単位に細かくプルリクする
- 例えば、レビュー者が変化を把握しやすい単位
- 例えば、レビュー者が変化をテストしやすい単位
- 単体・結合テストできる最小単位にしてみる
- 自動テスト (LaraveのUnitTest)
- 画面テスト (画面から登録できる)
- バリデーションのテスト (必須項目未入力だと登録できなくなっている)
- プルリクを分けるか悩んで、分けない事を選んでも、Commitは分けておく
- プルリクと工数見積がつながると良い
- 工数を見積もる時に、細かくタスク分割してから、工数を見積もると思うが、その細かいタスクがそのままプルリクになるのが理想
7. チームとしての活動
事前に準備するもの
- プルリクのルール
- プルリクテンプレート
- レビューのルール
- 期日
- レビュワーの最適人数
- レビューには工数がかかる
- 10人メンバーを増員しても、10人分のレビューをしたら、実質の開発速度は上がらない
- マネージャかチームでかレビューの最適人数を考えておく
- コーディングルール
- コーディングレギュレーション
- レギュレーションを更新するためのルールも決めておく
- 話し合ってアップデートする方式
- 一人でレギュレーションを管理する方式
- など
を用意
- 知識のレベルセット
- アーキテクチャの知識をチームへ浸透させる
- などなど
以上、ありがとうございます。