はじめに
最近久々にチーム開発をしました。
その現場ではがっつりプルリクのレビューを担当しているのですが、ここだけは守ってほしいなと思ったことを書きます。
コンパイルが通ることを確認する(動作確認できる状態にする)
そんなの当たり前だろと思うかもしれません。
しかし実際にあったんです、コンパイルの通らないプルリクのレビューを依頼されたことが。しかも何度も。
1回2回ぐらいであればまあそこまで気に留めません。とはいえ初歩的なミスには変わりありません。
プルリクはコードの品質を担保するためにあります。
逆に言えば担保できていればやる必要はありません。その時間を使ってがっつり開発すべきです。
しかし現実はそれが難しいから仕方なくやるわけです。
仕方なくやっていることに時間をかけたくありません。
それを「コンパイルができない」=「動作確認できる状態ではない」=「レビューするに値しないソースコード」、の状態でレビューを依頼されたら、
- 「これではレビューできないので直してください」
- 「わかりました」
という無駄なやりとりが発生するだけです。
相手の時間を使っているわけですから、相手になるべく負担を負わせない形で依頼するべきです。
相手の時間を使っているという意識があればこのようなミスは減らせると思います。
てか今すぐ無くしましょう。
レビュー内容が被っているプルリクを同時に出さない
プルリクAとプルリクBのレビュー依頼を受けました。
プルリクBはプルリクAからブランチを切っているので、プルリクBのレビューをしていて、「あれ？ここの実装プルリクAでも見たな」となるわけです。
プルリクAが一発でApproveすればAをマージできるので、プルリクBのレビューをした時にはプルリクAの差分は出ないので問題ないです。
しかしそうはならないことの方が多いので、プルリクAがApproveされた後にプルリクBを依頼するようレビューを受ける人がスケジューリングすべきです。
僕が遭遇したケースではプルリクBはAからブランチを切ったことをプルリクの概要欄にすら記載されていなかったので、危うく同じ指摘内容を送るところでした。
この問題も相手の時間を使っていることを意識していれば、負担にならないように依頼タイミングをスケジューリングしようと思うはずです。
気をつけましょう。
やっていないことを書く
チーム内のプルリクのテンプレートには大体「そのプルリクで対応したこと」という項目があると思います。
確かにスムーズにレビューに入れるように対応したことを書くことは必須だと思います。
それに加え、対応していないことを書くことも同じぐらい重要です。
また僕が遭遇した話になりますが、あるアプリの画面をまるっと実装するタスクをある方にお願いしました。
そのタスクにはAPIのリクエストの実装も含まれていました。
しかし、プルリクを確認したらAPIのリクエストの実装はされていませんでした。
なぜ実装していないのかと聞いたら、「API側が未実装だったから」とのことでした。
最初は「おおそうか、それなら仕方ないな」となりましたが、「いやいや、それだったら自らバックエンドのエンジニアと連絡取るなりしてでも進めろよ！」と後から思いました。
しかし裏を取ったわけではないので、バックエンドエンジニアと連携して精一杯やった上のアウトプットなのかもしれません。なのでそこまでは言いませんでした。
しかしなぜ実装していないのかは記載すべきです。
ここでも相手の時間がなんちゃらの話になりますが、こちら側としては当然実装してあるべきものがないので何で実装していないのか聞くわけです。そして返信が返ってくる。この一往復分のやりとりが無駄になります。
今はリモートワークの会社が増えているので、Slackがメインのコミュニケーションだという現場も少なくないです。
それなりにマネージメントの経験がある方は、連携がスムーズにいくよう言葉遣いにはかなり気をつけていると思います。
テキストでのやりとりは相手がどう受け取るのかわからないので、より一層文面には気をつけていると思います。
そんな中一往復でもやりとりが増えるとチリツモで結構精神的にきついと思います。
やっていないことを書くというのは習慣化して解決できる問題なので、徐々にチームに浸透させていってなるべくやり取りを減らすよう心がけていきたいです。
言わずもがなをやる
コードレビューの観点からは少しズレるのですが、そのプルリクで実装すべき内容はタスクに書かれていなくても実装してほしいです。
エンジニアは人によってレベルの差が激しい職種だと思います。
僕が見てきた中でも、言われたことをやるは当たり前でその上で改善のための実装を次々とやるスーパーエンジニアの方やそうでない人などたくさん見てきました。
そうでない方は技術力は低くないけど、とりあえず言われたことだけやっておくかという方もいました。
つまり何が言いたいかというと、人によって「DONE」のボーダーラインが違うということです。
となるとタスクをお願いする側とタスクを実装する側でDONEの認識がズレるから、プルリクで初めてそのずれに気づいて大量の指摘をする羽目になります。
じゃあタスクにやることをDONEの認識のずれがないレベルで全て書けばいいじゃないか！と思うかもしれませんが、大量の文章を書いている途中で思うはずです、
「これ書いてる暇があったら俺が実装した方が良くね？」と。
いろんなドキュメントがある中で矛盾がないように文章を書いてその後も更新していくのは大変骨が折れる作業です。
矛盾があったらあったで、認識を合わせるためのやりとりも発生します。
だからこそ「ここはこう実装すべきだろ、わかってくれよ」と思うんですよね。
僕が遭遇したケースでは、iOSのアプリの実装でUITextFieldのキーボードを表示したときに、UITextFieldがキーボードと重なってしまい見えないということがありました。
iOSエンジニアの方ならわかると思うのですが、コンテンツがキーボードと重ならないように実装するのって地味に大変なんですよね。
大変なのですが、入力中のテキストがキーボードで見えないとかUX的に大変よろしくないので、そこはタスクに書かなくても対応してくれよと思うわけです。
理想はそういう当たり前をチーム内で認識することです。
そのために最初は逐一言って浸透させていくしかないと思います。(リーダーが旗振ってくれるとかなり効果的だと思います)
誰でもわかるようにドキュメントを整備していくとかいう、後の管理コストが膨大になるであろうナンセンスなことはせずに、理想を目指していきましょう。
わからないことがあったら事前に聞く
「言わずもがな」をやるべきなのはわかったけど、タスクをこなす側としては「何が言わずもがななんだ？」と思うはずです。
最悪なのは勝手に解釈して先に進めることです。
「タスクにはこれだけしか書かれていないから、ここまで実装しておけばいいか！」
レビュー中にそんな声がソースコードから聴こえてきて悲しくなります。
僕がこれまで参画した案件では、不確定要素がなかった案件は一つもありませんでした。
エンジニアなら誰しも「わからないことがありながらも先に進める」という経験をしていると思います。
そのわからないことをどう解決するかが大事になるのですが、解決策は高い技術力だけではありません。
そもそも要件から外すか、もしくは要件に含めてもミニマムな機能に留めておく、というビジネスサイドでの決定が解決することもあります。
エンジニアはあくまでその案件の歯車の一人で、タスク内容も外的要因が決める場合があるということです。
ということは「言わずもがな」の実装も案件次第で変わってきます。
「このビジネスチャンスを逃したら何千万の損失だ！」みたいなスケジュールが絶対にずらせない案件の場合は「言わずもがな」の基準も当然低くなります。
その場合は「キーボードがコンテンツに重なっているからプルリク出せません！」みたいな悠長なことは言ってられません。その「言わずもがな」は案件が決めることです。
エンジニアは実装がメインの仕事なので、ビジネスサイドの話をキャッチアップできないこともあり、そういう「言わずもがな」の基準がわからないことがあります。(特に規模が大きい案件では)
だからこそ少しでも不明な点があれば聞いてほしいのです。
案件はスケジュール通りにいかないことの方が多いので、まるで子供に接するかのように「大丈夫？わからないところはない？」などといちいち聞いている暇はありません。
自分に振られたタスクでわからないところがあればわかる人に確認して、自ら前に進めてほしいです。
言われたことだけやるじゃなくて、一緒に案件を良い方向へ進めていこうという気概を持った人がいれば頼りになるし、自分もそうなりたいと思いました。
最後に
表題は良いプルリクを出そうというテーマですが、根底にあるのは「自分がどう行動したら相手が喜ぶか」ということに尽きます。
しかし、その行動がチーム内で良い方向に進められる行動なのかというのはしっかりチーム内で合わせておく必要があります。
案件が終わった後に酒でも飲みながら振り返って、みんなで良い方向に進んでいきましょう。