Closed18

【読書メモ】LGTM

木目壱心木目壱心

https://www.shuwasystem.co.jp/smp/book/b663838.html
LGTM本を読み始めました。
コードレビューの指南書です。
今日は1章部分だけなので前振りだけでした。


開発チーム全体で問題意識を持ってもらうための質問リストがあるんですが
最後の1個が「自信を持って、自分のプロセスを大好きだと言えるか?」でちょっと笑う、
とともに、感覚的に明らかに問題あることがわかる問いだと思う。

本書の対象者は「チーム開発をする(かもしれない)すべての人」としているので、私は完全にターゲットです。はい。


この本ちょっと誠実さを感じる部分があって

  • 説明する提案は万能策じゃないので、チームごとに判断しようね
  • PRの説明をちゃんとしてる(略:マージ前にコードレビューすること)

みたいな記述があって、全体としてなんか良さそう感がにじみ出ています。

ただ、絵文字を多用する癖があるのがちょっと違和感。

木目壱心木目壱心

2章に入りました

コードレビューの構造は3種類あると主張
対面でのレビューで進める「人間主導型」(レビュー記録が残りにくい)
ツールを介してリモートで進める「ツール支援型」(レビューが後回しになることがある)
その中間の「ハイブリッド型」(中庸、調整しやすい)
→会社でやるなら人間主導型〜ハイブリッド型、OSSならツール支援型になると思う

開発時間と予算のトレードオフになる(ツール支援型はサービス課金等が発生するから?)


体系的なコードレビューの仕組みを世界で初めて作ったのはIBMのマイケル・E・フェイガンらしい(1976年のこと)
対面の会議で3〜6人の人が250行ぐらいのコードをレビューする
コード作成者、それを詰めるレビュー担当者、ひたすらコード読解をする読者とたまにオブザーバーが入るらしい。
→役割が明文化されてるのはなんかTRPGっぽい

この手法で不具合が3分の2に減ったんだとか(割と残ってるような?こんなもん?)


このあと話はプルリクエスト/マージリクエストベースのレビューに移っていきますがそれはまた次回

木目壱心木目壱心

2章の続き

良いプルリクとは何か!(著者の意見)
①プルリクのタイトルで変更内容が大体わかる
②説明を読めば全部わかる
③外部ツールや文書を見に行く必要がないこと


タイトルのルール:
分類プレフィックス(feature, fix, docs, chore, breaking)をつける→変更の方向性が分かる
タイトルテキストは80文字程度(VT52基準、またかよ)
→※日本語なら40文字ぐらいにすべきだと思う
choreは雑用って意味(私の語彙になかった)
リンク切れのおそれがあるので、チケット番号をタイトルに入れるべきではない


説明のルール:
変更が発生し背景・根拠や、関連するチケット番号、影響を受ける機能やテスト手順を書く
→項目が多い、慣れるのに難しそう…

その他ルール:
プルリクにはタグを付ける(筆者はohmyzshのタグ分類推しらしい)
プルリクにはステータスを付ける(レビュー前なのかレビュー中なのかが分かるようにする)


記述ルールが細かい割に、それを強制するツールがないのがただただ不安
シンタックスエラーからでいいからちゃんと管理しやすくしたい…

木目壱心木目壱心

いいレビューをするために

  • エゴを捨てよ
  • 小さい問題はコメントで
  • 大きい問題は対面で
  • 指摘の根拠は明確に
  • 改善方針は具体的に
  • 流し読みしない、そうなっているなら改善すべき問題がある
    • テスターが見つけてくれると思うな
    • 自動テストの導入が手
  • レビュー時間は長くても1時間で

いいレビューをされるために

  • エゴを捨てよ
  • セルフチェックせよ
    • 面倒なら自動化(リンター、フォーマッター、…)
  • レビュー対象が少なくする
    • 意味のある最小の処理に絞る
    • 500行、20ファイルまで(先行研究がある)
  • わかりやすいレビューコメントを入れる(先項を参照)

いいチーム、管理者、組織になるために

  • コードレビューを文化にしろ
  • 改善提案を出しやすくせよ
    • 自動化を重視すべし
木目壱心木目壱心

3章に入りました
チームでコードレビューはすることになった。
で?コードレビューで解決したいことは何?というのが3.1です。
これを決めないと、「なんとなく」で始めたレビュー活動はただ手順を増やすだけに見えて意味を失います。


コードレビューで達成できる・改善できる事象として筆者は

  • バグを発見する
  • コードの安定性(プロジェクト内での一貫性というか)を高める
  • 知識移転・共有
  • メンタリング(=教育)
  • 作業記録

ただ重要なのは、どの作業も「コードレビュー以外の方法でも実現できる」ということ。
すでに他の方法で実現されているなら、コードレビューの目的に含めなくていい。
(たとえば、静的解析や自動テストでバグはある程度検出できる)


筆者がそれでも重視しているのは、知識移転・共有と記録を保持すること。

誰かが抜けたとしても、知識や情報が外部保存されていれば属人化を避けることができる。


余談
プロジェクトから抜けていい人数を表現する用語として、トラックナンバー(トラックにひかれても大丈夫な人数)は知ってたんだけど。
バケーション係数/バス係数がこの本ででてきた。同じ意味の用語増やすのやめて…

木目壱心木目壱心

LGTM本、3章の後半

コードレビューのツールを選び、導入してガイドラインを作り、ガイドラインを更新するまで


ツール選び

  • バージョン管理システムは何か
  • CI/CDできるか
  • セルフホストかWebホストか
    その他もろもろの条件を考慮して選ぶ。

GitHub, GitLabその他のツール比較表載ってたGerritがちょっと気になる


ガイドラインを作る

  • レビュー開始から承認までのプロセスはどうする?
  • どうレビュー依頼する?
  • 何をもって承認する?
  • レビューで重視するのは何?(整合性、規約遵守、ドキュメント?)
  • レビューを止める基準と止めない基準は何?(ちょとした誤字でリジェクトするかどうか)
  • 承認は誰がやる?(著者は2人以上を強く推奨)
  • ルール更新の仕方は?いつやる?(少なくとも年1回を推奨)
木目壱心木目壱心

4章、チームワークアグリーメント

チーム運営の規約文章(メンバーが共同編集可能であること)

  • プルリクエストの対応期間
  • レビュー基準
  • コーディング規約

などを明文化し、認識を合わせる


レビュアーによって基準がブレると、レビューされる側の納得感を損なってレビューの意味がなくなる(コメント無視される可能性が上がる)
だから基準を明確にすべきである。

人によって言うことが違うのを「社会なんてそんなもん」で片付ける風潮が嫌だった(実際そんなもんだとは思うものの、それを当然として改善しようとしないのが許せない)ので、この主張は使える。

木目壱心木目壱心

4章の後半
チームワークアグリーメントで決めたほうが良いこと

  • レビューの応答時間:チームによる 社内なら数時間だし 海をまたぐなら数日
  • プルリクの上限:20ファイル500行→多いならプルリクを分割
  • レビューで確認すること
    • 明らかな問題:リンターで検出可能、レビュー前に除去するのが望ましい
    • 予想される問題:リンターでは検出できない問題、コードの読みやすさや非効率な実装
    • 厄介な問題:実装以前の問題、前工程の見直しが必要。コードレビューの範囲外
  • プルリクの自己承認の禁止 と 自己承認をしても良い条件(緊急事態)の定義
  • レビュアーの好みに基づいた指摘(ニットピック)の禁止
  • 礼節ある態度の要求
  • ポリシー違反時の対応

そして、このポリシーは更新可能であること


暗黙の了解をなくし、すべてを文書化すること を著者は重視している様子。
→ レビューする/される姿勢ってあんまり明示されてないなー…
Everything as Code的な考え方に近い気がする。

木目壱心木目壱心

5章 自動化のメリット

知ってる or 汎用的ではない(GitHub固有)情報が多くてあんまり言うことはない

  • リント、フォーマット、静的解析はやっとけ
  • 自動テストにしとけ
  • レビュー文書のフォーマットは決めとけ(フォーマットに沿ってるかチェックするGitHub Actionがあるよ)
  • その他レビューに関する面倒なことはGitHub Actionで大体なんとかできるからいいの見つけようね

ぐらい(具体例を説明するせいでちょっと長いけど)


  • 自動化を実現するのにかかる手間/は評価するとか、自動化の基準は決めといたほうがいいと思う(同じ作業を3回したら自動化したほうがよさそう)
  • ドキュメントのフォーマットも自動化したい…(なんかいい方法ない…?)
木目壱心木目壱心

6章 いいレビューをするために

  • 客観的であること
  • 具体的であること
  • 結果、目的が明確であること

の客観的の部分


だめな例

  • レビューコメントで提案の理由が示されていない
  • Issueに乗せるべき内容をコメントしている
  • 変更すべき合理的理由が説明されていない(性能改善の程度など)

客観的なレビューをするために

  • 立ち止まる:レビューすべき内容かを考える時間を作る
  • 熟考する:なぜその提案をするのか自問自答する
  • 行動を決定する:提案、見送り、延期の判断をする

  • エゴを捨てる:レビューするのはコードであって人ではない
  • レビュアーがそのレビュー意図を明確に説明できること
  • レビュアーに質問しやすい環境であること
  • 必要な議論を惜しまないこと
木目壱心木目壱心

6章の「具体的であること」、「結果と目的が明確であること」の部分

  • レビューコメントに基づいて、修正が必要な不要かを明記する
    • レビューコメントを分類する(変更、根本的修正、軽微な修正、などなど)
    • 分類基準はメンバーで共通認識を持つこと
  • 複数箇所に同様の修正が発生するときは、すべての箇所に同じコメントを入れる
  • なぜ、どのように変更し、どうなっているべきかを明言する

攻撃性のない表現にする

  • 個人に対する批判にならないようにする(Youが多いと圧が強いらしい、主語を明記しないといけない英語の弱点では)
  • 1箇所は褒める(褒め過ぎはダメ、新人とかインターンには多めに褒めて自信をつけてもらう)
木目壱心木目壱心

7章「こんなコードレビュープロセスは嫌だ」

  • 形骸化したレビュー
    • なんかわかんないけどLGTM
    • このゲームには必勝法がある相互レビュー(相互承認)
    • 急ぎの修正 の常態化
  • 傷つけ合うチーム
    • 無駄にキツい表現
    • 人格批判
  • ダイナミックなレビュー対象
    • 連鎖するプルリク
    • 更新され続けるプルリク
  • ダルい手続き
    • プルリクに乗せる項目が多すぎ
    • 承認フローのボトルネックになってるメンバーがいる

→せっかく作ったレビュープロセスが無意味に…

木目壱心木目壱心

8章「レビュープロセスの改善案」

  • 負担は分散する
    • 特定個人にレビューを集中して割り当てない
    • チームメンバーが等しくプルリク承認権限を持つ
    • 前提知識がないプルリクから逃げず、学習の機会と捉える
  • プルリクの情報不足を補う
    • 更新内容が多い、不明確な場合はプルリクの作成者に説明させる
    • プルリクのフォーマットは遵守させる
  • 困難は分割する
    • 機能が大きい場合は設計段階から問題があったかもしれない
    • より細かい機能に分割し、プルリクをコンパクトにする
    • フィーチャーフラグの導入やgitのコミット整理(cherry-pickやrebase)も手
  • 複雑な議論は対面でする
    • 長文でチャットで議論するより、対面・通話で話してみる
  • 不慣れな人向けには作業を段階で細分化する
    • レビュー指摘が多くなりすぎる場合は、作業者の力量不足の可能性がある
    • 技術的、文化的な教育をする段階に立ち戻る
木目壱心木目壱心

9章「堅牢なレビュープロセスを作るために」

  • レビュープロセスを明文化する
  • レビュープロセスをチーム内の意見を取り入れて改善する
  • レビュー用の時間を確保する
    →前提に組み込むことで、省かれないようにする
  • ツールでプロセスを保護する
  • 開発手順を細分化する
    →プルリクを小さくする
  • 意見しやすい環境を作る
    →互いを警戒して無難なことしか言えない環境にしない
  • 数値化しやすい指標を過度に使わない
    →数値目標はハックしやすいため
  • 緊急事態の対応プロセスをあえて煩雑にする
    →緊急事態を悪用されないようにする
木目壱心木目壱心

10「緊急事態」
緊急時の対応をマニュアル化する。

  • プレイブック:手順書
  • ランブック:プレイブックを統合し、戦略をまとめたもの

緊急時対応プレイブックの要件

  • 詳細、簡潔、網羅的、わかりやすい、緊急対応の完了まで案内してくれる
  • 手順自体は面倒にし、悪用されないようにする

プレイブックの細かい話

  • ディシジョンツリーが含まれる。緊急時対応の条件を明確にする。(以下が例)
    • 違法状態
    • セキュリティインシデントの発生
    • 損失が発生中
  • 緊急時扱いする条件は可能な限り限定的にすること
  • 緊急時の判断を誰がするかを明記(管理者サイド)
  • レビュープロセスの簡略化内容(レビューなし、は極力回避)
  • 対応が完了したら、対応内容を文書化、報告し、事態を分析すること(意図的に複雑にしたい、再発防止のため)
木目壱心木目壱心

11章「ペアプログラミングの導入」

ペアプロをすればコードレビューは不要?
→得られるものが違うのでそうはいかない

ペアプロは2者間の知識移転が迅速化し、参画直後の立ち上げに適している
コードレビューよりかなり早い段階で指摘と修正ができ、方針転換がしやすい

その一方で、口頭でのコミュニケーションが中心になるため記録が残らないほか
ペア同士の視点が重複しすぎて見落としのリスクに繋がる
→コードレビューでカバーする

ペアプロは精神的負荷が大きいため、時間制限があった方が良い
また、メンバーの経歴や性格、作業内容で効果度合いが大きく変わるため 流動的な判断が必要

木目壱心木目壱心

12章「モブプログラミングの導入」
(非開発者を含めてもよい)3人以上のメンバーで行う開発活動

メリットとして

  • 複雑で不慣れな作業に効果的
  • 知識共有の範囲が広がる
  • 問題の早期発見

その一方で

  • 特定個人の意見に引っ張られる
  • 記録が残りにくい
  • 急に効果が出るものではない
  • (非開発者がいると)専門的な話が避けられがち

などの理由からコードレビューを置換する事はできず、補完する関係にあると著者は主張


人員コストが重い…
でも複数人で同じコード見ながらヤイヤイ言える場所は楽しそうではある

木目壱心木目壱心

13章「生成AIとの付き合い方」
書かれた時期によるものだと思うが、生成AIに関する認識がやや古い
(コードの読みやすさの観点でAIはレビューできない という趣旨の提言には反論したい)

総合的な主張としては「AIは開発作業の大きなコンテキストを把握していないので、過度に信頼してレビューをおろそかにするべきではない」、「信頼できる範囲で活用すれば、人間はより重要な作業に専念できる」といったところ。
的外れではないけど、時間経過で評価はいくらでも変わると思う。
AIを「単なるツール」(リンターと同じレベル)としてみる傾向にあり、「未熟な開発者」としているようには見えなかった。
(なので長期的に読む価値はなくなる章だと思う)

ふと思ったんですが、
すべての設計書や議事録、チャット履歴なんかを参照できるようにしたAIがあれば、人間以上に使える開発メンバーになる気がする。


総評としては、チーム開発においてコードレビューを導入する or 導入しているが品質にいい効果が出ていない 場合に向いている本だと思う。
※管理側への説得材料に使いやすい部分が含まれる。
また、コードだけでなく他の成果物に対しても同様に適用可能だと思われる。
(コーディング規約みたいなのが設計書やテスト仕様書にある必要があるが)

今回はあえて通読したが、目的に合わせて章をピンポイントに選んでも十分だと思う。(最後の生成AIの部分は読まなくていい)

このスクラップは10日前にクローズされました