Code Review
手戻りを減らす工夫
>#WIP
金言が詰まってる
試行錯誤の速度を上げることを重視しまくっている
数が多くてレビューが大変
詳しくない人がレビューし
ても品質が上がらない
>コードの土地勘は、結局レビューだけしてても得られない（と思う）
これ読みてえ
> 対ビジネスユーザーの問い合わせ
この辺の知見をまとめた書籍とかないのかな？
自動化できるところは自動化する
CIで、testや型エラー、lintなどの検証を行う
レビューする時に考えるべきこと
どれぐらいの時間をかけるべきか
どういう手順でコードを見ていくべきか
commitを追っていく
diffだけみる、全部見る
コードだけ見る、実際に動かす
段階が考えられそう？
まず型エラー・テストで確認
この辺はCIにやらせるべき
まず動かしてみてちゃんと動くか確認
プログラム内部を確認
↑上にあるほど、自明なミスなので早い段階で却下できる(?)
指摘の仕方、伝え方
「俺が修正したほうが速い」という場合にどうするか
レビューの観点
段階というか、分類がありそう
プログラムの変更容易姓の諸々
セキュリティ・パフォーマンスなど最低限必須なもの
汚いが動く、とりあえずユーザに価値は届けられる
仕様を満たしている
バグってない
レビュー後のコメントについて
最近「LGTM」しか言ってない
ここを見ました、ここは見てません、みたいなことをちゃんと言うべきかも
レビューをお願いする時に考えるべきこと
方針の段階でCode Reviewしてもらう
開発に着手する前に合意を取って手戻りを減らす
rebaseなどしてcommitを整形するか
λ git rebase developしてからpushするなど
どういうcommit粒度にすべきか
commit messageを気にする
PR前に一連のcommitを編集する方法が必要だなあ
fixupとかを駆使してキレイにしていく
どういうPR粒度にすべきか
例えば、リファクタ・フォーマットだけで1回PRを出す
PRには何を書くべきか
PRに書こう
レビューしてもらう側
こういうノリあるんだ、なるほど
>社内プロダクト開発においてはpull requestを出した当人がマージボタンを押す ref
> これは意見があると思うが個人的にはこだわりのスタイル
> 「自分が押したマージが本番に出ていく」という体験をしてもらう
> コードベースへの当人のオーナーシップの醸成
というか、GithubのレビューページでVSCode並のシンタックスハイライトや、コンパイルエラー、unusedの表示などをしてくれればよいのだが。
レビュアーの自動割当
レビューする時
主軸が誰にあるのかという話がある
自分が完全にマネージする場合は、かなり強めに必要なルールの徹底をお願いする
そうでない場合は基本提案になりそう
reviewerがコードにコメントを書くというのちょっとあり
書いている人は理解しているので、コメントの粒度の判断が難しい
記憶を忘れたときに自分でコメントを付ける必要がある
レビュワーが読んだときに、「ん？あ〜なるほど」となった箇所にちゃんとコメントを残すようにする
引っかかりポイントを減らす
今のチームは人数が少なすぎて、レビューの文化が薄いのであまり知見が溜まっていない
5つほどプロダクトがあって、エンジニアが2人なので、プロダクトごとに専属みたいになっている
だから、記事を読んだりして知見を溜めるしか無い
もっとOSSにcontributeしまくればいいかもしれない
でも、レビュワー側で参加することってあまりない
以下に書いてるのは、どこかの記事に書いてたやつで、良さそう、と思ったものをただ列挙したもの
良さそう、と思っただけで実践しているわけではない
レビューはできるだけ早く行う
特に日を跨いだり、手戻りがあるとだるい
参考になるリンクを張る
コメントにラベルを付ける ref
[must] 必ず対応してほしい
[imo] 自分の意見や提案・好み。 自分ならこう書く
[nits] 些細な指摘
インデントやタイポ
[ask] 質問、確認。
コードの意味や背景が分からないから教えて
レビューで指摘→直す→再レビューで新しい指摘、をできるだけ避ける
レビューされる側はだるく感じだろうな、というのはわかる
「1回で言えよ」と思うと思う
しかし、コードの実装場所がおかしいことに対するレビューと、仕様的な問題に対するレビューはわけないと話がしづらい
最高2回ぐらいかな、と思っておけばいいだろうか
あるいは1回目のレビューをする時点で、
「たぶん修正後にもう一度修正があるかもしれません。なぜなら、」と伝えておくとか
レビューする6このステップ ref
PR summaryを読んで意図を理解
CIする
テストが通ってない場合はレビューしない
通してもらってから確認する
commitログを1つずつ見ていく
commitログと、その変更
気になるところ、疑問があったらメモしておく
動かしてみる
気になるところとかを、確認する
全体のdiffを見る
GitHubにコメントする
伝え方の問題
すぐに正答例を示す
「自分で考えろ」からのダメ出しは心理的安全性が低いので避ける
だめだと思う理由を書く ref
「hogehogeはpiyoなので、良くない気がします」
具体例を提示する
こちらの方が良いと思う理由も添える
「私ならこうします」構文をつかう
アドバイスはするが、それに従わなくて良いというスタンス
テキストで指摘する
口頭だと、聞き取り手のメモが大変
良い感じの大きさにまとめたScpraboxを投げるのも良さそう
レビューの使い回しができる
レビューが終わったら、可能なら直接顔をあわせて言葉を交わす
テキストだけだと冷たい雰囲気になるので
なるほど
やんわりとした言葉で伝える ref
x あなたのメソッドの書き方は分かりにくいですよ。
o このメソッドは私には少々分かりにくいです。この変数にもっと良い名前をつけられないでしょうか。
少なくとも1つは肯定意見を伝える ref
こういうのも地味に大事なんだろうな
褒める
絵文字を使う
「これ読んで勉強し直してきて」はアンチパターン
上手くモチベートさせないといけない
「この本が参考になりました」とかは良いかも
でも別にレビュー時に言うことでもない気がする
別の機会の勉強会でそれとなく伝えるとか
レビューで指摘されることで、読むタイミングを作ることできる
本の内容がより頭に入るようになる
diffに現れない箇所も見る
バグを生みやすい箇所を重点的に見る
境界値検査とか
layer的に重要な箇所を重点的に見る
Entityとか、table構造とか
後から修正するのが大変な部分をしっかり見る
お金に関する処理とか
セキュリティ的に大丈夫か
法的に大丈夫か
UI、UXのチェック
初心者(?)にもレビューしてもらう
より詳しい人のコードを読んでもらう機会の創出になるので、プロダクトについての知識が増えるので良さそうではある
>意外と、レビュアーのコードがクソなことも多く、若手のメンバーのレビューはしっかり見てくれるため予期しない不具合を見つけてくれることも多く、実際的に有益です ref
例えば指摘項目がめちゃくちゃ多いときはどうするか
全部言う？小出しにする？
これは、たぶんそもそも事前に認識合わせをした方がいい気がする
ペアプロなりなんなり
あまりにも共有できていないと感じた時
実装前に方針を話しあう
ペアプロする
モブプロする
PR数を増やす
google
新卒に向けた資料
入ったメンバーがコードを理解するためのコードレビューだったり、
パフォーマンスならベンチマーク取るとか、
コードレビュー時に設計の問題が出てくるのはあまりよくなくて、実装前に共有しておくべき、
みたいな話がある
文化が共有されてないと辛そう
なんでいちいち指摘するの？時間かかるだけじゃん、とか言われると辛い
レビューは、技術的な好みを語る場ではない、という主張
はてぶがおおい
PRに100件もコメントが付くようなやつを、以下に短縮していくかの工夫を色々