ReadableなPull Requestを作る為に心がけていること
前田です。
何故かニックネームがダンプ前田になってしまいました。
昨日突然Slackに「ダンプ前田」リアクションが出来たからで、今プチ流行しています。
私がよくダンプファイルを作るかららしいです。
文章の接尾語に使ったり返事に使ったりなど、なかなかハイコンテキストな感じになっています。
弊社ではGitHubのPull Request(以下PR)機能を利用して機能単位ごとに必ずソースコードをレビューしています。
以前、弊社下條がソースコードレビュー時に意識していることとしてレビューア側目線の記事を書いていましたが、レビューイ側目線でリーダブルなPRを作る為に心がけていることについて書いてみようかと思います。
PRをリーダブルにするのが大事だと思っているのは以下の理由からです。
- レビュー時間の削減
- 実装した機能にフォーカス出来るのでバグや機能漏れを見つけ出しやすい
プロジェクトを進めていく時、機能単位で開発していくと思いますが、ソースコードを書いたらそこで終わりではなく、その後に必ずレビューをします。
そして、レビューによって、実装者には気づかなかったバグが見つかったり、仕様を誤認識していたり、などを修正し、品質を担保していきます。
プロダクトの品質を維持し高めていくためにレビューは必須ですが、レビューアが多いとそれだけ時間(コスト)がかかっていることになります。
コスト削減の為にもレビュー時間は少ないにこしたことはないと思います。
また、PRが散らかった感じになっていると、レビューして欲しいロジックにフォーカスしてもらえなかったり、そのことによってバグを見逃したり仕様が違ったままリリースしてしまったり、などということが発生する可能性が高くなると思います。
本質的な間違いを指摘してもらいやすいようなPRを作ることによって、質の高いディスカッションが出来るのではと思っています。
では以下から具体的に私が普段から心がけて実施していることを書いていきます。
実装背景を記載する
その機能がどのような経緯で必要になったか、詳細な仕様はどう定義されているか、などを記載します。
URLリンクなどを貼っておいて、後で見返した時に辿りやすくしておくのが良いと思います。
ただ実際には、GitHubはPRとIssueを紐付けることが出来るので、Issueにしっかり書いて、PRにはリンクだけ貼る、というパターンが結構多いです。
検証結果を記載する
PRは何か実現したい機能があって、それが期待通り実装されているかどうかが一番のポイントです。
私の場合はそれらを示す為に、ローカルの開発環境で検証した過程と結果などを示します。
具体的には以下のようなものを示したりしています。
- どのような検証データを準備したか
- どのような検証動作を行ったか(リクエストを投げたか)
- 期待する結果はどのような結果か
- 結果画面
データを準備する際のSQLを添付したりなどもしています。
レビューアがレビューする時は、コードを見るだけでなく実際に動かしてみてレビューすることがあると思いますが、自分が検証した過程を共有したほうが時間の節約になると思うからです。
ただし、上記はテストがしっかり書けていればそれほど親切に書いてあげる必要はないかと思います。
適度な粒度にする
ファイル変更数が膨大な数のPRがたまにあります。
私も昔はそんなPRを出したりしていました。
差分が大きいPRはレビューアの負担が大きいです。
どうしても大きくなってしまう場合もあるかと思いますが、無理にでも適切に機能を分割してPRを出していくようにしたほうがいいと思っています。
機能を適切に分割して実装していくのは難しいですが、慣れるしかないかなと思います。
-> PRを分割する時のtipsを追加しました。 ReadableなPull Requestを作る為に心がけていること②
リーダブルコードで書く
リーダブルにする為にはそもそもリーダブルなコードになっている必要があると思います。
最近は実装した後で自分のコードを見直す時間を結構多く取っています。
コードを見直す時の目線として、ここは説明コメントを付けないと分かりづらいロジックになっているなとか、説明コメントを自分で書いている場合、そのコメントは何故必要なのか、とか考えながら見直しています。
ビジネスロジックの背景などをコードにコメントとして残しておくのは良いかと思いますが、それ以外はすべてソースコードで表現出来る、もしくは出来ていない場合(説明が必要なコードになっている場合)は、何かロジックやオブジェクト名・メソッド名・クラスの定義の仕方・役割の持たせ方・分割方法などが悪いのではと疑ったほうがいいと思うからです。
説明が必要なロジックになっているようなら何か見直しが必要なサインだ、という目線で見て改善していくと、読みやすいコードになっていくのではと思っています。
リーダブルコードに書かれていたことが、最近ようやく肌感覚で分かってきた気がしています。
ここをこうしたい、ああしたい、と色々考えるとキリがなくなってくるのですが、読みやすいコードに出来たと思った時はとても気持ちがいいです。
間違いを指摘してもらいやすいようにPRを作る、というのはもしかしたら少し嫌な作業かもしれません。
チームギークで書かれていたことですが、
批判を耳にする側としては、それを受け入れる方法を学ばなければいけない。
自分のスキルに謙虚になるだけでなく、他の人があなた(とプロジェクト!)に恩恵をもたらしてくれると心から信頼し、自分がバカだと思わないことである。
プログラミングはスキルなので、練習すれば向上する。
ジャグリングの改善点を指摘されたときに、自分の性格や人間としての価値が攻撃されたと思うだろうか?
自分の価値を自分の書いたコードと結び付けてはいけない。
繰り返しになるが、君は君の書いたコードではない。
大事なことなので何度でも言うが、君は君の書いたコードではない。
君がそう思うだけでなく、同僚にもそう思ってもらうようにしよう。
にならって自分が書いたコードに対して謙虚に意見を受け止め、積極的に改善出来るようにしていく為にももっと「ReadableなPull Request」を作れるように努力していきます。
