0.はじめに
新人さん向けプルリクエスト（以下：PR)の送り方・受け方入門をざっと書きます。
仕事ではScalaをベースに使っているので、それベースにサンプルコードを書きます。
もし、こういうのもあったほうがいいんじゃね？というのがあったら、編集リクエストください。
吟味して、取り入れます。
また、あえて少し乱暴めに書きます。
なぜかというと、この初めてエンジニアになった人にあなたのそのミスはこれくらい上司が呆れたり、イラっとしたりしますよということを伝えるためです。
というわけで、新人さんは参考に、上司の方は教育の参考に使ってもらえると幸いです。
1.PRを出す前にチェックすること
1-1.環境編
まずは、最低限の話をする。
正直、この章の☑が全てとおらないモノを論外と思ってくれていい。
☑1: ビルドはとおるか？
これはホントに最低限。
一部の新人エンジニアを除いてグループ作業とかしたことないからわからないだろうが、（これから分かるだろうけれど）
チーム開発する上ではビルド通らない＝戦犯に等しい。
なぜならば、そのPRをマージしたら開発が止まってしまうからだ。
あと、正直いうと、上司はあなたのことなんて信用していない。
PRの差分だけみてということはまずない。
なので上司のPCであなたのソースが動かなければ、そもそも見る価値なんてないということだ。
特に注意：
新規プロジェクトの場合は、別フォルダーにgit cloneしてちゃんとビルドが通るか確かめてからだそう！
☑2: .gitignoreは適切か？（svnなら.svnignore）
最近、gitが浸透して学生時代にgit触ったことがある人が増えてきた。
とはいえ、学生時代にバージョン管理までして開発していた人は少ないかとは思う。
バージョン管理がわかんないなら上司に聞け。
もしくはこのあたりくまなく読むと幸せになる。
http://www.backlog.jp/git-guide/
んで、本題、.gitignoreは簡単にいうとバージョン管理から外すファイルの設定ファイルだ。
ここに以下のモノが入ってないか注意しておくと上司はスムーズにPRみてくれる。
- 中間生成ファイル
- ビルド後の実行ファイル
- MavenとかNugetとかで落とした外部ファイル
- .DS_Store (Macが勝手につくるファイル）
これらなぜ含めてはいけないか？
ソースをビルドしたら上司のPCにもできるその時にコミットしあったらコンフリクト誘発するからだ。
あと、ビルドしたら勝手にできるから無駄。
これは以下はチームによるが、含めないほうがいいだろう。
- IDEの個別設定
- プロジェクトの個別設定
個別設定については、あなたの環境と上司の環境は異なるからだ。
必ずしもあなたの環境が上司の環境と同じではないのだ。
☑3：コミットコメントは適切か？
上司はコミットコメントを見て、PRを吟味する。
自分がコミットした意図がかけているかをちゃんとチェックしよう！
とりあえず、これは論外な例をあげておく
git commit -a -m 'hoge' #何をしたかわからない辞めろ！
git commit -a -m 'ファイル追加' #commit log見たら分かる無駄なコメント、せめて何のためにファイル追加したのか書け！
git commit -a -m '途中までコミット' #git stash 使え！（svn はやむ無しなところはあるが）
git commit -a -m '(嘘の情報)' #嘘はつくな…
日本語能力を高めて、わかりやすいコミットコメントを書こう！
☑4: コミット漏れはないか？
新人にありがち、いや上司でもたまにやってしまうミス！
複数同時並行で作業してたり（新人に複数並行して任すのはどうかと思うが）、実験コードを書いたりしてるとやりがち。
まず、git pushする前に git status 叩け！
そうすれば、まだコミット漏れに気づくだろう。
何なら、赤文字のヤツをリバートしてビルドとか実行してみよう！
1-2.コード編
ここからはコード品質の話だ、1-1に比べたらAppendixな内容になるが、ここも最低限だと思って取り組んで欲しい。
☑1：コード1行ごとに理由を考えているか？
基本的に、上司は理解できないコードみると理由を聞く。
その時のために、そのコードを書いた理由を考えておくと上司も理解してくれる。
もちろん、ロジカルシンキングで考えておくとよい。
余裕があればピラミッドストラクチャーで強固なモノにしておくとさらによい
☑2：コードクローンしてないか？
コードクローンがわからないならググろう
ただそれも不親切なのでいうと、コピペだ。
コピペして一部改変して提出するパターン。
なんでこれが害悪かというと、もしコピー元にバグがあった場合コピー先もバグがないか調べないといけない。
ただ、コピペするパターンって一部改変するパターンが多い。
そうなってくるとコピペ抽出ツール（CCFinder）とかでも完全には検出してくれないことがある。
つまり何が言いたいか、コピペは諸刃の剣だということだ。
実装スピードはあがるがしかし、ベースがバグった場合にはバグの温床になる。
コピペするぐらいだったら、ベースの関数にテスト書いてベースの関数を使えるようにしよう
（プロジェクトとかソリューションまたぐ場合は除くが、本来ならライブラリ化して取り込むがベスト解）
☑3:代替となるライブラリが提供されてないか？
はっきり言おう。新人風情が考えたアルゴリズムなんてたかだか知れておる。
オープンソースにしろ、公式ライブラリにしろ熟練のエンジニアがチームでかかってライブラリ作ってるのだ。
敵うわけがない。
独自実装を強行する前にライブラリで提供してないか今一度確認しよう。
☑4：変数の名前は適切か？
実際にあったことを書く
val user = new UserDBAdapter
もう熟練のエンジニアならわかるであろう。
UserDBはDBアクセスが責務。
UserはUserとしてのデータが責務なのだ。
ありがちだが、まずは変数の責務をかんがえて変数名をつけよう。
Appndixだが、自然言語に近い変数名がつけることができれば、もうPR通ったも同然だ。
DSLを意識してかければなお素晴らしい。本当にできるのであればあなたはセンスがあると誇ってよい。
僕個人の意見として真理を書く。
プログラムは名前付けの科学なのだ。
たかだかローカル変数されどローカル変数。
上司はその名前付けのセンスでエンジニアの格付けをすると考えてよい。
もちろん、以下のような変数付は論外である。
val hoge = new UserDB
参考書籍としてリーダブルコードを読んで考えてみよう
Amazon:リーダブルコード
☑5: 【Web限定】レスポンスコードは適切か？
あるあるパターン
def getUser(useId:UserId) = {
try {
val user = UserDB().getById(userId)
Ok(html.index(user))
} catch {
case _:Throwable => Ok(html.error("データの取得に失敗しました"))
}
}
それ、ホントにOkか？
http://www.asahi-net.or.jp/~ax2s-kmtn/ref/status.html
ここあたりみて、適切なレスポンスコードを返そう。
（正直、Okでも許されるパターンはあるが…）
自分ならこう返す
def getUser(useId:UserId) = {
try {
val user = UserDB().getById(userId)
Ok(html.index(user))
} catch {
case _:Throwable => IntarnalServerError(html.error("データの取得に失敗しました"))
}
}
☑6: 例外もみ消ししてないか？
これもよくやるパターン
val user = Try(UserDB().getById(userId)).getOrElse(DefaultUser())
これは、本当はDBエラーがでていてアプリケーションを落とさないとサービス続行できないのにデフォルトのユーザー返して、破損した状態のまま継続している可能性がある。
この場合、DBエラーなのにログすら出ない。ヤバい。サービス続行の危機にまで行ってしまうことがある。
val user = Try(UserDB().getById(userId)).recover{
case e:UseDefaultUserException => DefaultUser()
}
もみ消していい例外ともみ消してはいけない例外をちゃんと区別しよう。
☑7：マジックナンバーはないか？
これも一般論だが、0,1以外の数値は上司にはわからないと思え！
def pages = (0 to 4).map(idx => Page(idx, ...))
まぁ、まだわかりやすいマジックナンバーを書いたが、4にも意味をつけてやるほうが更にわかりやすくなるだろう。
val MAX_PAGESIZE = 4
def pages = (0 to MAX_PAGESIZE).map(idx => Page(idx, ...)
これならば、あー最大のページ数が4ページだからそれでループ回してるのねってことに気づけるだろう。
☑8:Typoしてないか確認しよう
以外と初歩的なミスだが、自分が上司になってもやることがある。
主に、確認すべきは以下のところだ。
- チームのコーディング規約にそっているか？（ちゃんとCamelになってる？ちゃんとsnakeになってる？）
- 単語レベルで間違ってない？（MessageをMassageとか…）
こんな本質じゃないところ指摘したくない。
最低限、PR出す前に確認しておいてほしい。
☑9:Warningはできるだけ潰せ。
正直なところ、全部潰して欲しい。
ただし、一部例外があるからできるだけと書いた。
その例外はチームで確認はして欲しい。
あえて、例外的にWarningのこす場合にはWarning潰すチケット切るとかコメントうつとかで対処して欲しい。
☑10: 3日後の自分が読めるかを想像しろ
def hoge(useer:Seq[User]) = useer.map( x => (x.b , x.c)).map{
case (1, c) => (c + 3 , c + 4)
case (_, c) => (c +10 , c + 11)
}.foldLeft[Int](-5)((x,y) => x + y._1 + y._2)
def hoge2(user:Seq[User]) = user.map( x => (x.b , x.c)).map{
case (1, c) => (c + 5 , c + c)
case (_, c) => (c +10 , c + 11)
}.foldLeft[Int](-5)((x,y) => x + y._1 + y._2)
まさしく最悪な例を作った。
しかし、あなた方はこんなコード書かないよと笑っているかも知れないが、どれか一つの罠にハマってないか3日後の自分になって今一度確認して欲しい。
☑11：チケット（作業）のスコープから外れた修正はないか確認せよ。
正直、チケット外の修正が入っていたら見る方はつらい
なぜならば、チケットの内容をみてPRの妥当性を測るのがレビュアーの仕事だからだ。
だから、チケットのスコープから外れていたらリバートをかけてほしい。
もし、それでもチケット外の修正が紛れ込んでしまったらPRにチケット外の修正であることを明記しよう
（もちろん、原則紛れ込まさないのが前提だ。）
2.PRで指摘もらったら
2-1 修正するまえに
☑1：まずは指摘内容を理解するために5分は指摘を読め！
たいていの上司は、ぶっちゃけ新人の教育なんてめんどくさいと思っている。
世話焼きな自分でもたまにさじを投げたくなることが多い。(PRのコメント50超えたらさすがにつらい）
つまりは、PRの指摘もプロダクションコードならいざしらず、サンプルプロジェクトなら雑くなることが多い
まずはその雑な指摘を５分つかって噛み砕く努力をしたほうがいい。
☑2：5分で噛み砕いて、5分で指摘が妥当か吟味せよ！
吟味しないまま、修正に入るのは完全に愚行だ。
はっきり言う、上司も所詮、人間だ。ミスもする。間違いもする。ベスト解が出せてない場合もある。
もし、あなたの上司が、その界隈で有名人だったとしても上記の通り人間であることを忘れてはいけない。
あなた自身のジャッジをせねば、この先成長はない。
とある有名な言葉がある
trust but verify
日本語にすると「信頼はしている、だが確認はさせてもらう」
知らないのであれば、ググッてほしい。
チーム作業において、「信頼」と「確認」は別物だということだ。
☑3：10分指摘を噛み砕き吟味してわからなければ、指摘者に直接聞け！
PRをチームで見てる場合に限定されるが、直接指摘者に聞くことをおすすめする。
ありがちだが、怖い上司と優しい上司がいたらあなたはたいてい優しい上司に逃げるだろう。絶対にやってはいけない。
なぜならば、優しい上司は直接指摘した上司の意図を完全に汲み取ってるとは限らないのだ。
ヘタすると、直接指摘した上司の意図と外れたトンチンカンな解を示すことも時としてある。
だからこそ、指摘した本人に意図を聞くのがいいのだ。
コストの話までしたくないが、上記のミスをやらかすと、優しい上司の工数と直接指摘した上司の工数、そして二度手間をしたあなたの工数が食われるわけだ。
工数削減のためにも、直接指摘した本人の所に向かうべきである。
厳しいことを言うが、金もらってるのだ怖いとか優しいとか感情を廃して直接指摘者のところにいけ！
2-2. 修正のときに確認してほしい。
☑1：類似の間違いをしていないかさがせ！
はっきり言う、上司はプロダクションコードも抱えている。
だから、あなたのミスを全部見ることなどほぼほぼ不可能だと思ってほしい。
ましてや、同じようなミスをしているところを全て抽出してくれる上司などほぼいない。
なので、少なくとも1つ受けた指摘の類似箇所がないか、まず自分のPR全体をみよう。
Grepで探せるならGrepで見つけると幸せになれる。
☑2：過去の過ちは2度までにしよう！
ま、社会人になったら言われることだが、3度目同じミスをすると相当心象が悪い。
（とはいえ人間だからしょうが無い部分はあるのは事実だが…）
もし、コメントは99＋とかになっているのであれば、修正に対してかなり慎重に行った方が良い。
過去の指摘を見なおそう
それだけでも2度ミスは減るはずだ。
☑3: 修正理由は説明できるようにしておく！
前述の1-2 ☑1で書いたのと同様だ。
修正理由は説明できることが望ましい。
裏知識だが、上司はあ・え・て ベスト解を書かずに試す場合がある
その時に、上司の言うままに修正を加えると罠にかかるわけだ。
で、その時に訳にたつのが修正理由だ。
新人だものベスト解が思いつかないこともあるだろう。
だが理由を説明することでただただ罠にかかるのだけは避けて、上司に一糸報いたほうが上司の株があがる。
☑4：修正はYAGNIにそって
新人あるあるパターンだが、修正を見てると他の綻びも見えたりするわけだ。
その時に他の綻びを一切合切修正してしまう。
これは、愚行だ。
なぜか、上司は指摘を元にあなたの修正を見る、そこに変な修正がある → わからない。
まずは、指摘をただただ修正することに注力しよう。
それでも他の綻びも治したい。
ならば、少なくとも、他の綻びの修正はコミットを分けよう
2-3 修正終わったら
☑1：まずは修正をPushしたことをコメントをしよう。
当たり前だが、Pushしたら修正完了と思ったら大間違いだ。
修正完了とは指摘をうけ、指摘を修正し、修正を指摘者に確認してもらったら修正完了なのである。
なので、修正をPushしたことを上司に伝えてやる。少なくともこれくらいはやろう。
そこに、修正のコミットIDを載せてやるとなおよい。
私がいつも使うパターンだが**「XXXXXXX（コミットID）にて修正しました。」と書くだけでも随分ちがう。**
余談：
普段Stashを使っていて、メンションをつけることができる。
もし、直球で見せたい場合にはメンションをつけるのもありだろう。
☑2：感謝の気持ちを忘れずに…
ぶっちゃけ、これだけ書いておいていまさら精神論かくのも気が引けるが…
この気持なくすとPRがただただ辛いものとなるので特別書いた。
まず、上司は自分の作業時間を削ってあなたのPRを見てくれているのだ。
次に、上司はあなたのダメな部分を見つけて良くしてくれようとしているのだ。
(厳しい話だが、3年目あたりから明確に別れるが、正直こいつダメだとジャッジされたらコメントすら貰えない）
最後に、最初は卵だと思っているからこそ、他の先輩より厳しく見てくれているのだ。
(他の先輩にされる指摘よりも重箱の隅をつつくぐらいの指摘がくるのは優しさの現れなのだ。）
それらのことを踏まえて、「ご指導ありがとうございます」の精神を忘れないように。
余談：
この間のことだが、後輩からバグをみつけてくれるような指摘を受けた。
僕はさっそく彼に「ありがとう」と感謝を伝えた。
それぐらい、指摘を貰えることはありがたいことなのである。
3.マージする前に確認しよう
ここまできたあなたはやっとのことで修正が完了したのだろう。
しかし、仕事とは「勝って兜の尾をしめよ」というぐらい100点を求められる。
気を抜かずにマージまでたどり着いてほしい。
3-1.最終確認
☑1：マージ先のブランチが進んでいないか確認しよう！
マージ先がすすんでいた場合に、もしかしたらあなたのマージで壊れるかもしれない。
PR出す前にといいたいところだが、新人のPRの指摘対応は非常に時間がかかるのは上司は理解している。
なので、マージする前に以下コマンドにてマージ先のブランチを逆マージして動作確認しておこう。
git merge origin/master #マージ先がmasterの場合
これだけでもだいぶ、事故は防ぐことができる。
3-2. Conflictしたら…
☑1：自分で解消できるかどうか吟味せよ！
いつかは、必ず出くわすConflict。
SVN時代はマージ職人と言われるほどConflict解消は高騰スキルだったものだ。
はい、そうですね。高騰スキルです。
下手なマージはプロダクション壊します。
Conflict解消できないからスキルが低いことはない。迷わずヘルプを求めよう
もちろんのことだが、Conflict解消後にも再コンパイル・修正分の動作確認はしっかりして欲しい。
4. 最後に
新人時代にこんな文書を見ることができたら、どれだけ幸せであったであろうか？という気持ちで書きました。
熟練の方は「当たり前じゃん」と思うかも知れませんが、「あーこの☑忘れてたわー」という気付きもあると幸いです。
もう時期的に7月ですから、そろそろ新人がOJTなのか、本配属なのかわかりませんが、プロダクションコードに絡んでくるころでしょう。
その時のPRの指標の一つとして貰えれば幸いです。
長々と駄文を書き連ねましたが、ここまで読んでもらいありがとうございました。
また、何か思いついたら追記するかも知れません。
他の上司の方も編集リクエストいつでも受け付けておりますので、いろんな視点を頂いて、表題を【完成版】にしたいなと思います。
5. 付録
付録としてチェックリストをつけておきます。
読みながら使えるようにリンク付きです。
1.PRを出す前にチェックすること
1-1.環境編
- □ 1:ビルドはとおるか？
- □ 2:.gitignoreは適切か？（svnなら.svnignore）
- □ 3:コミットコメントは適切か？
- □ 4:コミット漏れはないか？
1-2.コード編
- □ 1:コード1行ごとに理由を考えているか？
- □ 2:コードクローンしてないか？
- □ 3:代替となるライブラリが提供されてないか？
- □ 4:変数の名前は適切か？
- □ 5:【Web限定】レスポンスコードは適切か？
- □ 6:例外もみ消ししてないか？
- □ 7:マジックナンバーはないか？
- □ 8:Typoしてないか確認しよう
- □ 9:Warningはできるだけ潰せ。
- □ 10:3日後の自分が読めるかを想像しろ
- □ 11:チケット（作業）のスコープから外れた修正はないか確認せよ。
2.PRで指摘もらったら
2-1 修正するまえに
2-2. 修正のときに確認してほしい。
- □ 1:類似の間違いをしていないかさがせ！
- □ 2:過去の過ちは2度までにしよう！
- □ 3:修正理由は説明できるようにしておく！
- □ 4:修正はYAGNIにそって
2-3 修正終わったら
- □ 1:まずは修正をPushしたことをコメントをしよう。
- □ 2:感謝の気持ちを忘れずに…
3.マージする前に確認しよう
3-1.最終確認
3-2. Conflictしたら…
6. その他
【参考】コードレビュー10年目がレビュアーとして、みてる部分をまとめてみた。
ようやく、自分のレビュー視点を言語化できたのでこちらも参考までに…
はじめて、コードレビューするとかいう方でもすぐに実践できる具体的なコード指摘をかき集めました。