#106 中級エンジニアのコードレビューは何を意識すべき!?
2023/1/4 ·
-
中級エンジニアになる条件なんだっけという話を
-
なんですけど40話でしたっけ41話かな話をしましたがコードレビューでそれっぽいコメントをできるようにみたいな観点があったかなと思いますありましたねコードレビューの話も1回やったんですけども別の観点でというところでGoogleのソフトウェアエンジニアリングっていう本があるんですがオライリーのドンキですねオライリーのドンキはい
-
その中にコードレビューのショーがあるのでGoogleのコードレビューの様子を見ながら中級っぽいコードレビューをしようという大丈夫ですかGoogleって全然最上級みたいな
-
あれですね脱菜磨きなんちゃらその先へみたいなそういうぐらい純米吟醸みたいな日本酒の話ですねいい日本酒の話をしてます今米削るみたいな上位ナンパかもしれないけど哲学は結局ねエッセンシャルというか非常に重要な部分を抑えてると思うのでちなみに言うと先に言うと絶対に真似できません
-
なぜならGoogle独自のツールを使ってからこそ成り立つフローとかでやってるんでまずそういうツールを作ってるんだって驚きがその1でもちょっとその辺の話は触れません難しくなっちゃうので駆け出しのコードレビューこうだよねGoogleはこうやってるよじゃあ中級はこのぐらいやろうかっていう話の流れができたらなとなるほどなるほどそこから導き出して決めていこうとはい
-
じゃあまずですね順番に聞こうかな行動レビューなんかどうやってやってます今っていうどうやって見てもらってるかって感じですかねそうだね見てもらうっていう観点も大事だね
-
僕がやることは基本ないのであれなんですけど作って見てもらう側しかないんですけど今の現場だと人数が少ないからっていうのもあるかもしれないですけど基本PGにおいてはプログラミングにおいては見てもらう人は一人しかいなくてその人と画面共有なり現場でデカいモニターで共有しながら見てもらってこここうだこうだって本当ワンオワンで
-
ついてもらって見てもらってるっていう感じでこのコードはいらないこの変数線形いらないんじゃないかとかここなんだろう分岐の部分とかいらないところとかの指摘をもらって直してまたすぐ見てもらってみたいな感じですねじゃああの
-
同期的なコードレビューが多いというか打ち合わせ形式というかそうですね基本一緒にやるのが同じチームの人というか全く同じプロジェクトをやっていてなんなら順平に仕事を振ってる人ぐらいの勢いが背景をそんなに説明しなくてもこのコードが何をしようとしてるのか全部わかってるよねみたいな人がレビューしてくれる感じだわりと俺が
-
そうですね俺がやるところに関してはもうすでに見てくれててみたいな感じでその人は基本プログラミングはしないんですよ僕ともう一人にこの機能この機能やってっていう風に言って出してみてもらうっていうだけの感じで最後の最後スケジュールとかに余裕がなさそうだったらでしゃばって手伝いますみたいなポジションで育ててくれてるっていう感じの
-
システムのことはやる部分は理解してるっていう感じの人ですもしかして過去のエピソードでじゅんぺいに後輩入ってくるよって言ってた2人のうちの1人の方ですかですじゅんぺいの後輩なんだやめてください働いてる歴はな聞かれるってやばいなこれ
-
ちょっとあの想像してたコードレビューと違かったんですけどそういえばそんなのあったわっていうのでちょっと若干面を食らってるんですけどでもめっちゃいいレビューだねなんかそれそういうの受けたかったわ最初の頃に1年目の時だけやりましたねそれボッコボコにされるやつやばフルボッコボッコボコにされるやつひどかったんで僕のコードもマジか最初からすごいコードの人なんていないでしょそりゃそうなんですけどねちょっとあの今日の話は
-
非同期の行動レビューの話をしますなるほどねプルリクを作って送ってレビューしておいてくださいお願いしますそうですそうですすいませんちょっと行動レビューって言って広かったですねのりさん今どんな感じでやること多いですか僕はもうその非同期のタイプですねプルリク作って見てもらう人に見てもらうっていう感じで別に見てもらう人も上司っていうよりも
-
本当に並行のチームメンバーみたいな相互レビュー的な感じですねプルリクのルールとかあります?例えばレビューを受ける側の人ってメッセージというかプルリクに書けるじゃないですか説明文みたいなあの辺こうするとかあります?あれは一応テンプレートは作ってて
-
チケットのリンクとやったことあえてやらなかったこと特に見てほしいことその他みたいな感じで入力欄がすでにあってそれに沿って作っていくって感じですかね素晴らしい見る側のルールってなんかあります?頑張る頑張るここに書く頑張る頑張るあんまないいいですね本当に思ったこと書くって感じですかねだからあんま決まったルールとかないですねうんうんうん
-
ありがとうございますノリさんのところまで行けたらいいなっていう話なんですが一方でGoogleどんな感じなんだろうっていうの拙いですがちょっとずつ話していきます気になるまずGoogleのコードレビューのフローですねGoogleなんかちょっと特集でですねGitHubじゃないツールを使ってますGitじゃないようにも見えました
-
実質GitなのでそこはGitだなって思って聞いてもらえればいいんですけどまず作者は変更のスナップショットって書いてあるんですけど変更したものと説明を作成してコードレビューツールにアップロードしますとアップロード時点ではコミットされてないらしいですコミットされてないんだただこの本で言ってるコミットが僕は読んでてマージに見えたので多分
-
実質Gitでいうプルリクなのかなと思ってます作者は自分の変更をレビューしてもらいたかったらレビュアー1人以上に連絡してコメントを依頼しますGoogleは基本的にレビュアーは1人最小限にしましょうっていう原則というか文化があるらしいですレビュアーはそのコードレビューツールを開いてコメントしてきますと
-
フィードバックに基づいて変更修正してOKだったらLooksGoodToMeのタグを付けるらしいです聞いたのちょっと前のやつだちょっと前のいいねみたいなやつそれでLGTMが付いたらLooksGoodToMeが付いたらコミットを許されるっていうのがGoogleのフローなんですが細かいところで言うとレビューは24時間以内にせよとか
-
それ大事大事ですよねマジで後回しになることあるからねそうなんですよ変更は小さくしろ200行まで行数決まってんのあとは変更の説明もフォーマットがやっぱり決まっててまず何が変更されてるのかそれは何で変更されてるのかっていうのを書きましょうというルールがあったりとかレビュアーなんですけどGoogle
-
にも限らないかもしれないですがコード作るときって何かのシステムとかにプッシュするじゃないですかそのシステムを司る人がいるらしいんですよどうやらGoogleってそれぞれのシステムになんかプロダクトオーナーみたいなというよりはOSS作った人ぐらいのイメージですねプロダクトオーナーというよりは
-
だから本当にGoogleの中で動いているというか作られているソフトウェアって本当にOSSみたいな形で管理されているらしくてめちゃめちゃいろんなツールがある中でそのツールの神みたいな人たちがそのプッシュに対して絶対レビューしてどんどん機能を更新していくっていうところがあるみたいですなるほどね本当OSSっぽい普通無理じゃないですかそんなこと何ですんでしょう
-
さばけなくないですかあの規模だとさらにやばくないっていうそれもねとんでもないけど彼らならできるんでしょうね変更小さくしろって話があったと思うんですけどGoogleのコード変更の35%はコミットというかプルリークか35%は一つのファイルに対するものらしいです完結する
-
してるんだってもしくはめっちゃちっちゃくしてるってことなのかもしれないですけどこれもとんでもない別に無理やり詰め込んだからそうなったわけじゃないですよね確かにそういうわけじゃないらしいですよ35%なんで一番多いってわけじゃないんですけど35%は結構多いと思います
-
確かにすごいですよねっていうような形でフローとしてはそういう風にやってるらしいんですがそこで文化を育てるっていうのをやっぱり大事にしてるみたいでコードはちゃんと書いた人もプロフェッショナルだからこの書き方よりこっちの方が絶対に決まってんだろバーカみたいなコメントはせずまず質問しましょうみたいなどういう意図で
-
こういう書き方にしたのか指摘は基本的にはドンっていう指摘じゃなくてディスカッションベースでコメントしましょうねっていうのがあるみたいですねその中でどっちがいいかを決めていこうと基本的にリスペクトを持ってっていうのは書いてますねGoogleのエンジニアなので言うてもすごいハイレベルな戦いをしてるんでしょうね多分そうなんでしょうきっと
-
っていうような感じですねフローだけでフローとか細かい文化で言うとそういう風にやってるよっていう話でしたここから僕は学び取ったことがあってコードレビュー中級エンジニアならこれくらいやれよっていう提案をさせてくださいポイントは受ける側とする側それしかいないよね受ける側とする側ですまず受ける側レビューE一つ
-
うわーこれレビューしたくねーっていう量のプルリクをしてはならないすいませんでしたはいしてはならないすいませんでしたえーとまあ行数で言うとどんぐらいかって言うとむずいんですが1000はダメだよねとかどんぐらいなんだろうなでも行数じゃないですけど20とか30ファイルが入っているのは作ったこと全然ありますはいそういうのとかもやめた方がいい
-
僕もやっちゃいますテストとか書くとテストファイルとかテストコード俺テストコードは関数ごとに分けちゃうんでテストコードというかテストファイルかとかそういうのも分割してプッシュした方がいいなと思いました基本的にフィーチャーGitFlowだと機能ごとにブランチ切ると思うんですけど
-
これが正しいやり方というかベストプラクティスではないかもしれませんがググるとエピックブランチ作ってフィーチャーブランチ切るみたいなことをやってるところがあるらしくてどういうことかというと一つの機能をもう一個多段に分けてもっと細かくプルリク出すみたいな
-
形をすることでレビューしやすいプルリクを出せるように工夫しているプロジェクトとかもあるみたいですエピックブランチっていうのは一時的な幹ブランチみたいな感じですかそうです幹ブランチってのは極太の枝を一個切ってそこからさらに小枝作るみたいなイメージですね多分そうですそうですまずそのプルリクのサイズは気を使いましょうちゃんとレビューはもう人間です
-
先輩でも大変なのは見れませんし人間だからたくさんポカンってくるとミスります見れるようにしましょうが一つ二つコメント説明フルリクの説明ちゃんと書きましょうのりさんが言ってたのではすごくいいなと思っててまずチケットでタスク管理してるんだったらこれは何のタスクに関わるフルリクですっていうのをまず最初にポンって書きます変更した
-
内容これを実装しましたでその意図はこうです意図はこうだからこういうことはやってません例えばエラー処理は別チケットにあるから今回やってないよとかですね僕もう一個これがあるといいんじゃないかっていうのは見ていただきたい観点に近いんですけどこうやったら動かせるぜ手元でっていう動作方法操作手順操作手順みたいな
-
ブランチ引っ張ってきてそのまま動いたらそれでいいんですけどそのまま動くにしてもコードの実行コマンドを張ってあげるとかしてあげると優しいかなと思います確かにレビュアーへの思いやりってことですねそうですレビュアーも人間ですからボコボコに言ってくるかもしれないけどっていうのが基本的にレビュー側に必要な気遣いなんじゃないかなと思ってます動機レビューは話が変わってくるんですけどね確かに
-
レビューすると気持ちわかるようになると思うんだけどマジで知らん話を急にされるぐらいのスイッチングコストありません?レビューするときってめっちゃある若干研修とかでもレビューみたいなのしてたんですけど確かに自分の関わってない機能を見るときにだいたいプロリクもらってだいたい話に行ってましたねこれどういうこと?どういうことって
-
分かんないのでだいぶそうですねツイッチングコストある感じはします分からない時に聞きに行くのは正しいからそれはそれでいいけどそうしなくても伝われればもっといいとは言い切らんけどそれはそれでいいからねちゃんと気使ってプルリクしましょうこれはまずレビュー側の
-
中級エンジニアはこうあれのレビュー側って思ってるんですけどどうです?全く異論ないです異論なし異論なしです暇プロ的中級エンジニアのレビュー側こうやればまず小さくプルリクしましょうチケット概要意図動作方法をプルリクに書きましょううんうん
-
思いやり思いやり大事小さく分かりやすく次レビュアー側これも非常に難しいんですがこれは完全に独断と偏見でいきますねまず一つ
-
24時間以内にデビューしましょうそれなーこれ超大事ですさっき出てきてましたねこれあのGoogleがやってるって言っててそれなーって思ったんでそれだーって思います分かるわーそれ何日も放置してしまうやつあるーそうなんですねフルリクを放置することによってチームの生産性が非常に下がりますね下がるわー確かにうーん
-
だってさ3日放置されてさプロリークするじゃん3日放置されてさ修正コメント来た日にはさ早よ言えやって思うもんなんだっけとかってなってなんだっけってないですね本人も忘れるか非常に効率が悪いなので頑張って24時間以内に返しましょう無理だったらごめん無理って言いましょうちゃんともしくは残業しましょうそうですね
-
悪い顔してるきついな2つコメントのタグをうまく使ってコメントしましょうですねコメントのタグ前のプルリクのエピソードにもちょっと言ったかもしれませんけどノリさんが言ってたんですけどコメントにこのコメントはどういう意図で喋ってるよっていうタグをつける文化が一部あるんですよ例えば
-
絶対修正してほしいっていうコメントはマストってつけるとかあーはいはいはいただなんかいいねみたいなのはシーってコメントっていうタグつけるとかそれちょっと正直チーム内のコミュニケーションはもちろん必要ですこのタグどういう意味ってなることはあると思うので確かに合わせつつですけどこれはどういう意図で言ってるのかっていうのを明記してあげてコメントするとレビュー受ける側が
-
嫌な気持ちにならなかったりするのでちゃんとその伝えられるようにタグ大事だと思いますあれする側にもいいですよそうなんですかコメントの中でもさうわこれ絶対やってほしいってやつと
-
こっちの方がいいんじゃないかなっていうやつとレベル感あるじゃないですかありますねっていうのが一色体になった時に自分の方が先輩だった時相手が全部それを丸ごと飲み込んじゃう可能性あるじゃないですかありますねっていうのはやってほしくないなっていうことがあるんである程度そこはレベル分けしていけると出す側もやりやすいなと思いますねレビューをする側かコメントしやすくなりますよねなる
-
in my opinionっていうタグつけるんですよねそうIMOあくまで俺だったらこうするけどどっちでもいいよみたいなそうそうそうそうどうぐらいのやつかもしれないですねでそうですよね受け取り側もどうしても変えてほしいのかそうじゃないのかが分かりやすくなるっていうタグ打ちましょう次最後ですね最後
-
リスペクトを持って強くいかないようにしようですさっきのにも近いんですけど絶対こうだろうじゃなくてここにこうなってる意図は何ですかみたいな引っかかったところにはちゃんと聞いてあげるなぜなら作ったのはその人でレビュアーよりも多分向き合ってる時間が長いので見えてるものが多い可能性があると後輩だったとしても確かに確かに
-
それでこういう観点で間違えてるのねだったら適切にアドバイスできるかもしれないですねっていう観点でちゃんとコメントできるっていうのが中級イズムだなと確かにね普通にあるもんねコメントしてあれこれこうなってるけどどういう意図みたいな感じで意図分かんなくて意図聞いたら確かにこれじゃないと無理じゃんみたいな発見は多々あるそうですよねうん
-
ひょっとしたらこうやってやろうと思ったけどどうしてもできなくてこうなってるとかもあるかもしれない確かにねそれはありそうだねライブラリの問題でみたいなそうなんだつってなのでちゃんと話を聞いてあげるスタンス本当に対人間ですよね本当だね気持ちいいメンタルなんだテクニックは本当一部って感じだわそうですねお互いの思いやり
-
プログラミングしてると忘れますからねコード書いててコード書いててコメントもなんか丁寧に書かないじゃないですかなんて言うんでしょう言い回し的な意味で明確に分かりやすく書くじゃないですかそのノリでプリキュア書いちゃうとちょっと怖くなっちゃうんで確かに普通にコミュニケーション取るようにって言うと難しいんですけど気使いながらできるといいんじゃないかなというのがレビュア側ですですね
-
全く異論なしということで優しくしてくださいじゅんぺいいつレビューになるんだろうねどうなんすかねそろそろあるんじゃない1年くらいないんですかまだ次の現場入ってもちろんまた下っ端で入るわけですからでも下っ端でもやれってなるよえー相互レビューの場合はなるよ
-
偉いからやるわけじゃないからね偉いというかできる人にレビューが集まっちゃうからその原理だとそんなことにはならないから確かに相互レビューとかありそうだな人数多いと普通にあるんじゃない?あるかな相互レビューじゃなかったことあんまないなたまたまかもしれないけどこういった現場はレビューがなかったですねすごくない?どういうことなんそれなんで動いてるのそれテストはあったけど
-
いやすごいなテストを信用しすぎてるなすごいよねすごいでも動いてたんですもんね動いてたそういうとこもあんだなマジで動いてたすげーよマジですごいなんか知らんけど動いてた動いてたすごいやーはいレビューあるかもしれないなはいぜひ思いやりを伸びしろにはなるんでね絶対レビュー確かに人のコード見るのもやっぱためになるからねそうですねこう書くんだとかありますよねうんマジでいまだにちょっと落ち込むもんなあー細か
-
レビューしてて発想のとこやっぱありますよね思いつきっていうんですか効率よく書くとかクリーンコードとか読んでも思いつかないことは思いつかないんで経験ですねっていうので中期エンジニアになるためのコードレビューでそれっぽいことをするレビューいい側だったらちゃんと丁寧に説明してクルリックも小さくまとめてあげると小さく作って
-
丁寧にレビュアー側だったらちゃんとタグつけて押し付けがましくないコメントをすると大人の対応24時間以内に大人の対応をするとこれができたらあなたも中堅じゃないです素晴らしいでもすごいしっくりきましたしっくりきましょうしっくりきましたね
-
Googleのソフトウェアエンジニアリングの話はちょっとしかできなかったんですけどうまくGoogleとしては本当にあの会社って仕組みで解決する会社なのでさっきのコードレビューのルールとかフローがガチガチに決まってるんですよで
-
会社によってはノリさんのところもそうですし僕のところもコードレビューのテンプレートがあったりとかルールは少しちょっとあるんですけどそういうのをちゃんと
-
チームとしてやることでこういう恩恵があるよっていうのを書いてたので最後にそれを紹介して締めますまとまりがいいですねコードレビューをちゃんとするとこんな良いことあるよ全部で6つです1つコードの正しさがちゃんとチェックできます2つコードの変更が他のエンジニアにとって意味が把握できるものであるということを保証することができます
-
どういうことかというと書いたコードは誰でも理解できるものになっているのが確定しますちゃんとレビューされているから知らない人が読んでも分かるコードになっている3つ目コードベース全体での一貫性を強制することができますコードニングルールこれ守ってないやんってプルリクで言えるからです4つチームのオーナーシップ
-
を心理的に促進することができます主体的に動くみたいなところ総合レビューで機能的に動いていると他のコードもちゃんと見なきゃとかシステム前提としてこうなっているべきだからあなたのコードこうじゃないよねみたいなそういうのを育てられますよ5つ目知識共有を可能にしますこれもレビューすることによって学ぶことがありますしねそうだね
-
最後コードレビュー自体の履歴の記録ができるようになりますプルリクに誰がレビューしたとかって残るじゃないですかコメントとかでコメントでアップルーブしましたみたいなというのでちゃんとコードレビューしてるしこの機能作ったの誰レビューしたの誰というのも後で追いますよというので結局品質がどんどん担保できているというか状況は作れるのかなと思いますというのが開始なので
-
この辺なんか課題あるわという人そんなチームはコードレビュー今一度見直してみてはいかがでしょうかというところでこれは見直してぜひともね綺麗なプロダクトにしていってほしいですよねそうですねちょっとペアプロに近いかもね感覚というかメリットの部分というかそうですねペアプロやるほど時間ないけど
-
難しい効果欲しいみたいな時にコードレベルしっかりやるってのはめっちゃ良さそうに思いましたね各々が自立してればできますねペアプロは片方自立してなくてもできるんでそういう使い分けはあるかもしれないですねペアプロやりたいんですよね分かる今度やろうと思って今年中にやろうと思ってるわそういえば忘れてた思い出した
-
今年度中にやろうと思ってたびっくりした仕事をおさめしたって聞いたから仕事をおさめしたんですけど今やってるプロジェクトでやれるからやろうと思って言ってたのを今思い出しました年末年始で忘れる気しかしませんメモっとけリマインド設定しておきます
-
という感じで終わりましょうかOK以上行動レビューの話でしたバイバイ
#106 中級エンジニアのコードレビューは何を意識すべき!?