ハローワードプレス、開発部のオジサンエンジニアことDJです。「おっちゃん」は別に構わないけど、「おっさん」とは言わないでください。
今回はプルリクエスト（プルリク）についてのお話です。プルリクエスト・通称プルリクって何？コレはGIT自体の機能ではなく【GitHub】【BitBucket】【GitLab.com】【Backlog】等gitホスティングサービスの機能です。
上記のホスティングサービスを使わない・使いたくない・（プロジェクト都合でインターネットに接続できないなどの理由で）使えない場合はソレ用のサーバー借りて、【GitLab】【GitBucket】をインストールするのもアリです。
アイキャッチ画像に書いたのですが、個人的には「プルリクエスト」と言うより「マージリクエスト」って言う方がシックリきます。「GitLab」では「マージリクエスト」という名称らしいですね…。
?では「プルリクエスト」ってどんな機能？
ざっとどんな機能かという事を説明させていただきますと、下記の画像の通りプルリクエストの編集内容をチェックし、マージしても良いのか、まだ改修が必要なのかを判断するためのレビュー機能です。要は「こちらのブランチのこのコミットのマージをお願いします！」って機能なので「マージリクエスト」って言った方がシックリくるような気がすると書いたのはこのためです。
図ではマスターから派生したブランチをマスターにマージするイメージですが、マスターからデベロップブランチを派生して、デベロップブランチから開発用のブランチＣを派生して開発に使用したブランチＣをデベロップブランチにマージ（融合）させたりプロジェクトにより様々です。親ブランチから子ブランチを派生して、子ブランチから孫ブランチを派生して、孫ブランチから子ブランチへ融合させ、子ブランチから親ブランチに融合させて、完了となります。
コードレビュアーも図ではマスターブランチの管理者がレビューするように見えますが、プロジェクトによっては開発者ＢがブランチＡのプルリクをレビューしたりその逆もあったり様々です。開発者がＤ，Ｅ，Ｆの３人であればＤのコードをＥ/Ｆで、ＥのコードをＦ/Ｄで、ＦのコードをＤ/Ｅでレビューというケースもアリですね。
✋で、レビューするのはどんな点？
まず、「読みやすい！読みにくい！」は個人の感想に近いのでレビューの際の感情とは切り離します。
○○ラーメンは美味しい。✖✖カレーは不味い。?これは「美味い」か「不味い」か食べる人が決める事です。ソースコードも同様に読む人によっては読みやすく感じたり、読みにくく感じたりします。要は読みやすくなるように書くのではなく、規約に沿って書くこと。これにより秩序が保たれ、結果的に可読性を上げることになります。大切なのはルールに沿った書き方がなされているか？今後の保守性も考慮に入れた書き方がされているか？障害を誘発する書き方はされていないか？などです。ソースはプロジェクト内のメンバーの共有財産です。規約を無視した書き方、障害を誘発しそうな書き方をしたブランチをマージされると困りますよね？「カオス」というのはそういう意味で個性に満ちたソースが混在してしまい、どれに準えて書けばいいのか分からなくなっている状態です。
どんな点をチェックするのか見ていきましょう。
?1.コーディング規約をチェック
if($a == 1){
++$s;
print ‘$a : 1’.”\n”;
}
?カッコ前後のスペースとインデント（字下げ）についてPSR-2 コーディングガイドを参考にしましょう。ifの次のカッコの前後にはスペースを一個入れる。カッコ内のステートメントはスペース4個で字下げする。とはありますが、既存コードとの前後関係もありますので規約を頑なに守り続けるというより、柔軟に既存コードに馴染むような書き方をした方が良いかと思います。既存のコードがタブインデントであれば、合わせてタブでインデントしておく。インクリメント演算子「＋＋」は前置にするか？後置にするか？なども個別の取り決めがあったりしますので、周囲のコードを読むと同時に空気を読んで馴染ませるような心がけを忘れずに。
【PHP】PSR-2 Coding Style Guide（コーディングスタイルガイド）
if ($a == 1) {
（半角スペース*4）$s++;
（半角スペース*4）print ‘$a : 1’.”\n”;
}
?ifの次のカッコは前後に半角スペースを入れる。＋＋インクリメント演算子は後置にする（周辺のコードが前置なら前置に合わせる）。インデントはスペース4個にする（周辺のコードがタブインデントなら合わせてタブインデントにしておく）。
//java風の関数宣言か？
if ($a == 1)
{
（タブ）++$s;
（タブ）print ‘$a : 1’.”\n”;
}
?個性を主張するなら、自分のマーキングっぽく足跡残しには良いかも知れませんが、浮いてしまう分だけ後続の改修担当者が統一性の無さに困惑してしまうでしょうね。書いた本人も年月を経れば自分内のルールは変遷したりします。忘れてしまえば、統一性の無さにどちらにしようか迷う所です。
改行コードがlinux改行（LFのみ）かwindows改行（LF＋CR）かmac改行（CRのみ）かも確認しましょう。
?2.コメントアウトの過不足をチェック
//変数$aに1を代入する ?見れば明らかなので、このコメントアウトは不要。
$a = 1;
/*** 「変数xを変数yで割った余り」を変数zに代入 ***/
$z = ($x%$y);
/*
?これも見れば明らか。これ自体より、これを得たことにより何をやりたいのか？どんな影響があるのか？むしろ、そっちが知りたい！
*/
?これは極端な例ですが、見れば分かる事なのでコメントアウトは不要です。複数行を束ねて「初期化処理」etc..で十分なところです。10行~20行,30行となってくると、その塊として何をするための処理なのかを説明する必要はあると感じます。
ポイントは「動作の説明」ではなく「何をするための処理なのか？」「これをする事で後々に何をやりたいのか？」を書くと後続の改修担当も間違いに気づけます。「あれ？コメントに書いてあることをやりたいらしいけど、この書き方では○○な場合に動かなくなるリスクがあるなぁ…。そんなときに発生するかもしれない障害を回避するためにも書き換えておこう！」みたいにです。
?3.変数名、関数名が適切かをチェック
$b = count($articles);
$c = count($tags);
?$bと$cでは下の行で使いまわすときに、記事の個数とタグの個数という事が分からず都度都度この行を見返す羽目になります。
$article_count = count($articles);
$tag_count = count($tags);
?一例としてですが、このように変数名に意味を持たせれば前の行を探して何をするための変数なのか調べる必要はなくなります。（$article_count→記事の個数、$tag_count→タグの個数。）
対面で説明をするときにも、「変数b」だの「変数c」だの言うより「article_count」や「tag_count」という方が伝わりやすいですよね？
function get_sum ($a, $b) {
（半角スペース*4）return ($a + $b);
}
$total_count = get_sum($apple_count, $orange_count);
?この関数名は単なるsumでもOK（人によっては第一引数のアドレスを受け取り、第二引数の値を足すのかな？と思ってしまう可能性もあります…。set_sumと混同するかも？）ですが、get_sumとすることで和が得られるんであろうな…というニュアンスです。この関数が使われていれば、宣言された箇所を読み返さずとも「おそらく、和が返ってくるんだろうな？」という推測が出来ます（多分、りんごの個数とみかんの個数の合計を代入するんだろうな…。みたいに）。尚、このときの変数名および関数名は「get_sum（スネークケース）」にすべきなのか「getSum（キャメルケース）」にすべきなのかは前後関係を読みながら合わせましょう。真っ新（まっさら）からコーディングできるならどちらでも構いません。一度決めたその規約を崩さなければOKです。後続の開発担当がそのルールに準えてコーディングしてくれることが期待できます。
参考：「キャメルケース」と「スネークケース」の違い
https://wa3.i-3-i.info/diff71moji.html
＞基本的には、先人の慣習に従います。
＞どちらでも良さそうな場合はキャメルケースを使っています。理由は単純で、キャメルケースの方が（「_」を入力しない分）打つ文字の量が少なくて済むからです。
?の方は打つ文字数が少なくなる「キャメルケース」が好きなようですが、ワタシは大文字小文字（シフトキー）を使わずに「_（アンダーバー）←これはシフトキーを使いますが…」で単語を区切る「スネークケース」が好きです。慣習のためかjqueryを書くときは「キャメルケース」の方が好きですね…。そういう点でどちらがナチュラルに馴染むか？という点で心境が変わるかもしれません。
?4.問題を発生させる可能性をチェック
//国別で挨拶をセットするセクション。
if ($lang == ‘JP’) $morning = ‘ohayo!!’;
?何が問題なのか？書いた時点では問題ないかもしれませんが、「$afternoon = ‘konnichiwa!!’;（午後には”こんにちは！！”をセットする）」を追加したくなった時、どこに入れますか？行の後ろに追加？次の行に追加？
if ($lang == ‘JP’) $morning = ‘ohayo!!’;$afternoon = ‘konnichiwa!!’;
//あるいは
if ($lang == ‘JP’) $morning = ‘ohayo!!’;
$afternoon = ‘konnichiwa!!’;
?上下の二つともlangがJP以外でも「$afternoon = ‘konnichiwa!!’;」が実行されてしまいます。この書き方ではifが効くのは次のステートメントだけで、さらに次のステートメントはifが効かず実行されてしまいます。ではどうするか？
//パターン1
if ($lang == ‘JP’) {$morning = ‘ohayo!!’;}
//パターン2
if ($lang == 'JP') {
$morning = 'ohayo!!';
}
//パターン3
$morning = ($lang == 'JP' ? 'ohayo!!' : $morning);
?パターン1,パターン2なら追加する箇所は迷うことなく出来そうです。
//パターン1
if ($lang == 'JP') {$morning = 'ohayo!!';$afternoon = 'konnichiwa!!';}
//パターン2
if ($lang == 'JP') {
$morning = 'ohayo!!';
$afternoon = ‘konnichiwa!!’;
}
//パターン3
$morning = ($lang == 'JP' ? 'ohayo!!' : $morning);
$afternoon = ($lang == 'JP' ? ‘konnichiwa!!’ : $afternoon );
?パターン1のような書き方はあまり見ません。なぜなら「1.コーディング規約をチェック」で上げました「PSR-2」ではこのような書き方ではないからです。パターン3は当初の一行で収めたいという気持ちは守られますが、メンテが煩雑になりそうです。という点でオーソドックスにパターン2の書き方がメンテ性も高まります。せいぜい一文のためではありますが、ifのカッコで括るのが無難そうです。
?5.ロジックの適正処理・適正箇所をチェック
例えば、サニタイジング処理については1.SQLのサニタイジング（「”」→「\”」、「%」→「\%」など…）と2.HTMLのサニタイジング（「’」→「’」、「<」→「<」など…）がありますが、MVC型のフレームワークで開発する際にはSQLのサニタイジング処理はM（Model）クラスで実行し、HTMLのサニタイジング処理はV（View）クラスで実行するのが一般的ですし、多くのプロジェクトではそれに準えているかと思われます。
?※ここで発見！！word_pressのバグなのか『「’」→「＆ａｍｐ；＃０３９；」』（半角では表示がうまく行かないので全角に変換しました。）と書いたところが『「’」→「’」』になってしまっています。心の中で置き換えてご覧ください。
それに反し、C（Controller）クラスで実行すると保守性が低下したり、DBのテーブルにおかしなデータが入ったり、ブラウザを通して見たときにおかしな文字が付加されてしまったりします。（そのリスクが高まります。）下記は当社とは全く関係ないサイトですが、気になったので例として挙げます。
?赤で囲った個所、「宇宙人“E.T.\”との物語が」❓❓（ダブルクォーテーションの前にバックスラッシュがある！！）
SQLでのサニタイジング処理がHTML内にまで引きずっているのでしょうか？このような事象が発生したりします。
コレ以外にも日付＆時間の処理はそれを処理するためのクラス/モジュール、「テキストファイルからの読み込み/テキストファイルへの書き込み」をするためのモジュールなどがフレームワーク自体にも備わっているかと思いますので、個別でCクラスにダラダラと書かないようにします。（「出来る」と「適任」「向いている」は違います。）
?6.疑問点は質問をしてみましょう
質問をしてみると1.改修の際の間違いかもしれませんし、2.意図的にやっている事かもしれません。意図的にやっている事なら学び取ることも可能です。
//タイトル、本文などのバリデーション処理。どれかに引っかかったら移行の判定は不要なとき。
$ret = true;
if ($ret == true) {
if ($title == “”) {
$ret = false:
}
}
if ($ret == true) {
if ($body == “”) {
$ret = false:
}
}
?ワタシが結構やる事なのですが、1行目で$retにtrueを入れて、2行目でtrueか判定しますが、2行目で判定する意味ないような気がしますよね？何故、こんな書き方するのかと言うとtitleチェックの前に別のチェックを入れたくなったり、bodyと順番を逆にしたくなったりするときもネスティングをいじる必要が無いからです。
$ret = true; ?この次行に別のチェックを入れたくなる時があるかもしれない。
if ($ret == true) {
if ($title == “”) {
$ret = false:
}
}
if ($ret == true) {
if ($body == “”) {
$ret = false:
}
}
?nit-picker（ニット・ピッカー）という存在
nitpickerとは…
- 細かいことにうるさい人
- つまらぬあら探しをする人
- nit-pick〔【語源】シラミ(nit)を拾う(pick)〕
https://www.ei-navi.jp/dictionary/content/nitpicker/
小さくて不当な批判をする人
何となくですが、伝わりますでしょうか？（特に開発でなくても、こういう方いらっしゃいますよね？）例を挙げます。
$total = ($yukichi * $n + $hideyo * $m);
?演算の優先順序について、解釈間違いが無いように
$total = (($yukichi * $n) + ($hideyo * $m));
?「掛け算にかっこを付けた方が良い！」とか、逆に「付ける必要が無いなら付けない方が良い！」とか、どちらでも良いような事に指摘をする人かと思います。一万円札がn枚、千円札がm枚あるなら合計でいくらになるか？掛け算と足し算の優先順位なんて分かります。括弧は、有っても無くてもどちらでもいいです。算数が分かれば明らかですよね？
?プルリクのレビューで一番難しいのは良い点を探したうえで褒める事！
ここまではチェックする箇所・方法について見てきましたが、指摘するよりも難しいのは褒めるポイントを探して褒める事！誰だってマイナスポイントを指摘されるだけで褒められることが無ければ日に日に萎えてきます…。
そんな気にさせないように褒めるのは大切な事です。（プラスポイントを指摘する！）
✋では、どんなポイントを褒めるんですか？
自分と思惑が一致した時とか、自分が知らない関数を使いこなしている！とか、自分が思いつかないやり方をしていたら褒めるポイントです。
たとえばですが、「自分だったらこう編集するであろうな…」という目論見がプルリク作成者とほぼ同じ内容であれば「思惑が一致していますね。私もこれがベストかな？と思います！」とか、自分の予想を超える改修方法であれば「あ！こんな手もあったんですね。今後はコチラのやり方を参考にさせて頂きます！」とかです。
?WIPでプルリクを作る！
WIPとはwork in progressの略で「進行中」「やりかけ」という意味です。
プルリクを作っておけば他の人にレビューしてもらうことが出来るのでマージ段階まで来なくてもWIPコメント追加しておけば「あぁ、まだ作業中なんだな…」と理解してもらえます。
?作成したプルリクはセルフチェックするのが一番効果的！
他人のコードチェックや自分のコードをチェックしてもらう点について書いてきましたが、何より一番効果があるのはセルフレビューです。自分が作成したプルリクを見返すことで「あれ、こんな改修を入れたっけかなぁ？？」「あ、この部分はテストのために追加したコードだ！消し忘れてた！」などに気付けば手戻りを防ぐことが出来ます。
プルリクのお話についてはこのくらいにして、次回のネタはネット検索（ネットサーフィン）の仕方についてお話ししようかと思います。
ググる。ヤフる。インフォシクる。?これらのコツです。
ありがとうございました。次回もよろしくお願いします。
文：開発部DJ
