学生インターンの初Pull Requestがようやく通った話 #PHP

ITベンチャーでエンジニアをメインに働く私が大学４年生の学生インターンを教育することになった。
自社サービスのコピーサイトを作るという課題を丸投げしてみて、
インターンからの初めてのプルリクをレビューし、ようやくマージまでこぎつけたので、その経緯と所感を綴る。

プログラム経験は大学で少しCを触った程度だが、非常に優秀で初めて触るPHPも自分で調べ解決していけるインターン君なので、自主性を重んじようと課題の与え方としてはほぼ丸投げだった。

TL;DR.

課題を丸投げしたが優秀な学生だったため自力でPRを作るまでこぎつけた。
コードとしては指摘箇所が多かったため、指摘すべき部分を分類して小分けに指摘していった。
指摘箇所はすべて細かいルールを知っているか否かでしかなかった。
現状、教育が成功していると捉えているが、改善の余地は大いにある。

Pull Requestの内容

PHPのフレームワークLaravel5.5で、自社サービスのコピーサイトを作ると言う課題に対して、
初めて投げられたPRの内容は、下記のものだった。
- TOPページで都道府県をリンクとして一覧表示する
- 都道府県のリンクの遷移先では、都道府県内の市区町村の一覧を表示する

当然、表示機能だけではなくこれらに関わるmigrationやシーディングも含まれる。

レビュー方針の決定

まずは初めてPRが作られレビュー依頼がきたことに喜んだ。
なにせ、GitHubの使い方もあまり教えず、「自分で調べてPR作ってみて」と投げていたほどだったので...。
また、動作確認をしても、意図通りの動作をしている。これも喜ばしい。

しかし、コードレビューをするとこちらはひどい。
悪いところがありすぎて書き出せない。

だが幸い、悪いところはすべて細かいことだった。
教えられてもいないのにそこまで完ぺきにできないのは当たり前。

とは言え、数が多すぎたので1回のレビューではコメントしきれない。
出来たとしてもダメ出しの多さに落ち込まれてしまう可能性もある。

ここでレビューの方針を決めた。
指摘すべき部分を分類して、そのカテゴリ毎に下記の順で行っていく。
① 可読性について
② migration(テーブル構成とシーディング)について
③ Modelについて
④ Controllerについて

実際にこのカテゴリ通りにきっちりとレビューを分けられたわけでは無いが、
順に指摘していき、レビューは7回に及んだ。
各レビューの指摘内容を記載する。

レビュー:1~3回目(①可読性について)

今回僕は、
コードの可読性(読みやすさ)についての指摘をしました。

他のエンジニアや将来のインターンくんが、
このコードに手を加える時、コードが読みやすければそれだけ作業スピードも上がります。
いいエンジニアになるためにはコードを読みやすく出来ることも大切なことなので今のうちから頑張ってみてください。

なお、今回のレビューでは同じように改善出来る箇所が複数あったとしても基本的には1回しかコメントしていません。
コメントのない箇所も他所で指摘された改善ができるかどうかを考えながら全体の修正を行ってください。

と説明した上で、下記の内容でコードの可読性について指摘した。

コードの整形について
- 配列定義などの際のカンマの後ろにはスペースを入れる。
- 変数の定義が複数行続いている場合は、イコールの位置を揃える。
- 連想配列の定義時や引数として連想配列を使う時は、1行1要素にし、ダブルアローは揃える。
- 配列定義などの際のカンマの後ろにはスペースを入れる。
- Viewファイル上でも、foreachなどがあった場合はインデントする。（ただしこれはデザイナーとの全面戦争を引き起こす可能性がある。）
ディレクトリやファイル、変数やmethodの命名について
- 変数名に省略形を使わない。
- Viewのディレクトリ名とファイル名は、それぞれController名とmethod名と対応させる。
- Controllerには、フレームワークで予め決められたmethod名があるためそれを使う。

可読性についてなので当たり前ではあるが、
①(フレームワークの仕様も含めて)コードを書く際のルールを知っているか
②わかりやすく書くように心がけているか否か
の2点でしかない。

レビュー:4回目(② migrationについて)

可読性の次はテーブル関連の指摘。
当然ここがしっかりしていないと、ModelやControllerの修正に移れない。

テーブル構成
- 変数名同様、意味を考えてカラム名をつける
  - 市区町村テーブルcities_tableの市区町村名カラムがcityになっていた。
- タイムスタンプ用のカラムcreated_atとupdated_atを作る
- 同様にソフトデリート用のカラムdeleted_atを作る
シーディング
- idは自動連番で入ってくるので、指定不要。
- タイムスタンプ用のカラムに、値を挿入する際には、DatetimeではなくCarbonを使う

カラム名の件で何故そうしたのか疑問が残るが、今回の指摘も知識があるか否かの問題がほとんど。

レビュー:5回目(③ Modelについて)

Model
- リレーションにおいて多のデータを取得するmethod名は複数形にする
  - 都道府県Modelクラスにて、市区町村Modelへリレーションを定義するmethodがcity()になっていた。

ほぼ可読性の問題。

レビュー:6回目(④ Controllerについて)

Controller
- Controller内で取得したModelオブジェクトは、リレーションが定義されているModelのコレクションへのアクセスが可能なので、Viewへ変数を渡す際に後者を別の値として定義することは不要。
  - あるViewに都道府県のModelオブジェクト$prefectureとその配下の市区町村Modelコレクション$prefecture->citiesを別々の変数として渡していた。

こちらも知識の問題。

レビュー:7回目

LGTM

所感

指摘部分のほぼ全てが、知っているか否かつまり、知識の問題だった。
知識の問題と言う言葉で片付けようと思えば全て片付いてしまう。
だが、バグを起こしていたわけでもないし(簡単な機能なのでおこしようもないが)、
彼の考え方や処理の仕方が悪かった部分は一つもなかったと思っている。

確かに知識がありさえすれば、レビュー回数が7回に及ぶこともその分の時間をかけることもなかった。
可読性の教育を事前にしていれば、少なくとも3回分のレビューはなくせたかもしれない。

しかしながら、可読性についての知識をつけるような座学的な教育よりは、
自分で動くものを作るという実践的かつ成功体験がつめる教育をしたかった。

この意味で、課題を丸投げした今回の教育は現状では成功だといえる。

ただ、今回の成功は彼の高い学習能力や粘り強さに寄るところが大きい。
彼のような性質を持っていないインターン生には通用しないやり方だろう。

コード規約、フレームワークの仕様、データベース設計などの知識を座学的につける教育方法も準備していきたい。