こんにちは！
スマレジ・テックファームのWebエンジニアやまてと申します。
はじめに
今回は『プログラミング研修でのフルボッコなコードレビューの記録』と題しまして、昨年12月1日に入社からの2カ月で受けた社内の研修について振り返っていきたいと思います。
同時期にWebエンジニアになった方々（他社の）が
「実務でコードレビューしてもらってフルボッコになっている」
と言っていて、本人たちは大変そうですが、実装スキルについては爆伸びしていそうでそれはそれで羨ましいです。
また、「他の方が初期にどのような注意を受けているか聞いてみたい」とも言っていたので、私は自分の書いたコードをレビューしてもらった機会はまだ研修のみではありますが、どのような点をレビューで教えてもらったかについて、自身の振り返りを兼ねてピックアップしていきたいと思います。
正直、「フルボッコ」という言葉が面白くて使っているだけなので、私がもらったコードレビューにフルボッコの要素はなく、ただ丁寧にレビューしていただいた記録です。
まず、スマレジ・テックファームの研修についてですが、実務未経験で採用された際には、SESでの出向先が決まるまで、社内の教育担当のWebエンジニアの方から研修をしてもらえます。
研修内容は、Web開発全般についての基礎的な研修で、以下のカリキュラムでした。
これらの内容についての課題を解いていき、 GitLab で マージリクエスト して、エンジニアの方にレビューしてもらう、という流れで実施しました。
本記事もカリキュラムに沿って振り返っていきます。
① PHPの基礎
まずはPHPの基礎についての課題についてのコードレビューです。
例えば下記のような課題です。
「この商品リストを連想配列として定義し、それぞれについて「〇〇(商品名) は △△(価格)円です。」と出力してください。」
このように配列や、その他条件分岐やクラスメソッドなどの基礎文法の理解を問う問題が20問ほどあり、OKもらえるまで修正しました。
それでは一部抜粋して、項目ごとにまとめたコードレビューを振り返っていきます。
1. 読解力
1.1.「出題された要件のとおりに回答するようにしてください」
まずはプログラミングですら無いですが、要件のとおりに実装できていないという指摘をいくつかいただきました。要件のとおりに実装するためには、読解力が当然必要になりますね…。
2. リーダブルコード
2.1.「変数名の区別がつきづらいため、どちらかわかるように記載してください」
例えば、「税込価格や税抜価格が出てくる場合、変数名で明確に区別をつけるようにしてください」と、レビューで指摘をいただきました。他にもクラス名など、名付けるものには色々と正確に表現するよう指摘をいただきました。
2.2.「処理の順序がわかりにくいので、処理手順を、日本語のコメントで記載してください」
コメントを書くことで、処理の順序がわかりやすくなっているかを振り返りやすくなりました。また、コメントを書く場合は、コメント自体が分かりにくくなっていないか、という観点も大事だと思います。
2.3.「計算する場所、表示する場所など、固めて書くようにしてください」
オブジェクト指向型は使っていない手続き型で書いた回答（コード）に対してですが、色々とバラバラになっていたため、レビューで指摘をいただきました。「読む人にとってのわかりやすさを考えましょう」という指摘を何度もいただきました。
フルボッコ例
課題
「CSV形式の以下内容のファイルを読み込んで、１行読み込むごとに商品名と価格（税抜）を画面に表示するプログラムを作ってください。また、※箇所のように価格（税抜）が無い行がある場合、例外処理を行って、画面に「エラーが発生しました」と表示し、そこで処理を中断するようにしてください。」
| 商品名 | 価格（税抜） | |
|---|---|---|
| 牛肉 | 300 | |
| じゃがいも | 200 | |
| 玉ねぎ | 200 | |
| 醤油 | 300 | |
| 本みりん | 0 | ※ |
| 砂糖 | 200 |
答案コード
<?php /** * 「商品名」もしくは「価格（税抜）」が無い行がある場合、例外処理する * * @param array $data * @return bool */ function judgeTwoValue(array $data):bool { try { // 値が2つ(商品名,価格（税抜）)揃っていない場合にエラーとする if (count($data) !== 2) { throw new Exception(); } return true; } catch (Exception $e) { // 画面に「エラーが発生しました」と表示する echo 'エラーが発生しました'; // 処理を中断する exit; } } // 1. 01-08-00.csvファイルを読み込みのみでオープンする if (($handle = fopen("csv/01-09-00.csv", "r")) !== false) { // 2. 表示する echo '<pre>'; // １行読み込むごとに商品名と価格（税抜）を画面に表示する // (「商品名」もしくは「価格（税抜）」が無い行がある場合、例外処理する) while (($data = fgetcsv($handle)) !== false) { if (judgeTwoValue($data)) { echo $data[0] . ',' . $data[1] . PHP_EOL; } } echo '</pre>'; // 3. オープンされたファイルポインタをクローズする fclose($handle); }
レビュー
- カラムが２つか判定するメソッドは、本当にメソッド化する必要あるか。問題点としては、
- メソッド内で、画面に表示する処理があるので、プログラムが読みにくくなっている。最初に言った通り、表示する処理は固めて書いてほしい。
- メソッド内に処理が中断する箇所（exit）がある。これも処理が読みにくくなるので、メソッド内では極力中断しないようにしてほしい。
- ループ内で1回づつ、キャッチする必要はない。
3. PHP
3.1.「while文では何もしない場合、それをわかるようにしてください」
<?php while (($data = fgetcsv($handle)) !== false) { $count++; if ($count === 1) { // 1行目（ヘッダ）の場合、合計金額の計算をせずに次の行に進む
この問題については、continue;を用いて「1行目の場合は現在の繰り返し処理を終了し、次の繰り返し処理に進む」ように書くと、このwhile文では1行目は何もしないことが分かりやすくなると思います。（書いていないと、他の人が見た時に意図したものなのか、書き忘れなのか分からないため）一概に「これが正解！」と言うことはできない場合もありますが、重要なのは他の人が読んだ際にも分かりやすいか、という観点です。…というレビューをいただきました。
3.2.「foreachループが終わっても$itemは最後の要素を参照したままなので、unset()しておいてください」
<?php // 食料品カテゴリーの商品の価格を2割引した連想配列を生成する function createDiscountedFoodsArray(array $items):array { $discountedFoods = []; foreach ($items as $item) { if ($item['カテゴリー'] === '食料品') { $item['価格'] = (int)round($item['価格'] * DISCOUNT_RATE, 0); $discountedFoods[] = $item; } } unset($item); return $discountedFoods; }
foreach ループを終えた後でも、 $value は配列の最後の要素を参照したままとなります。 unset() でその参照を解除しておくようにしましょう。 さもないと、次のような目に遭うことになるでしょう。
PHP公式の「さもないと」が怖いので、習慣づけるようにします。
2022/08/24追記
上記のように書きましたが、 &（リファレンス） を使わなければ unset は不要と知る機会がありました。
ループの中で配列の要素を直接変更したい場合は、
$valueの前に & をつけます。こうすると、変数には リファレンス が代入されることになります。
<?php $arr = array(1, 2, 3, 4); foreach ($arr as &$value) { $value = $value * 2; } // $arr は array(2, 4, 6, 8) となります unset($value); // 最後の要素への参照を解除します ?>
と公式ドキュメントに例があるとおり、 & をつけて参照しているときは、「foreachループが終わっても$itemは最後の要素を参照したままなので、unset()しておいてください」ということになります。
（追記ここまで）
3.3.「この配列に無い商品はItemクラスのインスタンスとして生成できないので、定数で商品の配列を定義しないようにしてください」
<?php /** * 商品クラス */ class Item { // 商品のリストを定数として定義する public const ITEMS =[
オブジェクト指向、全然分かってなかったので、基本的な指摘をたくさんいただきました。
ここで定数で商品の配列を定義してしまうと、この配列に無い商品はItemクラスのインスタンスとして生成できません。「商品名」「単価（税抜）」「適用税率」を引数に持つコンストラクタを定義すると、ここで定数で配列を定義する必要がなくなります。という旨のレビューをいただきました。
3.4.「型の自働変換を常に意識してください」
<?php // 1. 01-08-00.csvファイルを読み込む if (($handle = fopen("csv/01-08-00.csv", "r")) !== false) { // 2. 1行目のヘッダを取得する $header = fgetcsv($handle); // 3. 価格（税抜）の合計を計算する $totalPrice = 0; while (($data = fgetcsv($handle)) !== false) { $totalPrice += $data[1];
ここでの$data[1]は 数値を表す文字列(string型) です。
例えば2行目で牛肉の値段を足す時は、
$totalPrice = 0 + "300";
となりますが、PHPではこの演算の際に自動的に型を変換しています。
$totalPrice = 300;
これは常に意識しておくと良いと思います。
と教えていただきました。
3.5.「標準関数が用意されているものを使ってください」
<?php // 4. 表示する echo '<pre>'; foreach ($nameColumn as $name) { // １行目に商品名をカンマ区切りで表示する echo $name; // 最後の要素ではないとき if (next($nameColumn)) { echo ","; } else { echo PHP_EOL; } } unset($name); foreach ($priceColumn as $price) {
PHPには、配列の要素を指定した文字列で連結する標準関数が用意されていますので、そちらを使うよう修正お願いします。
と、レビューいただいて、implodeを使用して修正しました。
4. その他
4.1.「PHPDocを書いてみましょう」
研修中は以下のような決まりで書きました。現場によって決まってるから、社内での決まりを確認してから書くように、とのことでした。
クラス
/** * クラスの概要／要約 * * @auth <開発者名> * @version バージョン 日付 */
変数名
/* @var 型 変数名 */
メソッド名
/** * メソッドの概要／要約 * * @param 型 変数名 説明 * @return 型 説明 * @throws 例外をスローする場合書く (発生する場合) */
4.2.「CSVってなんですか？」
夕会（週2回、17:00に進捗確認の会を開いていただいてました）で、「用語や実装を言葉で説明してみて」とよく指導いただきました。「ちゃうっ」「何言ってるかわからない」と、ほぼ確実に言われましたが…笑
人に説明して伝える、という機会は多くあるので、慣れておくように、とのことでした。
ちなみに、
CSVとはComma-Separated Valuesの略で、カンマ区切りで並べた値という意味です。
自分の言葉で伝えられる、というのが本当に理解した状態だな、と思うので、自分の理解の浅さが歯痒いですが、日々向き合って脳みそに刷り込んでいくしかないかなと思います。
それにしても丁寧なレビューをいただいて、振り返りながらも感謝の念が湧きました…
今回の記事は以上です！
次回、「プログラミング研修でのフルボッコなコードレビューの記録 - ② データベースの基礎」投稿予定です。
はじめてのSES出向、今日で丸3カ月経過。これまでやってきたのは、使用するWebAPIの仕様確認、フロー図や表作成、Doker開発環境の構築、既存のソースコードの修正箇所の確認など。引き続き、経験20年のエンジニアのリーダーの考え方を学びつつ、目の前の自分のできることを一つずつ全力でやっていく。
— やまて｜Webエンジニア2年目 (@r_yamate) 2022年6月14日
3ヶ月経つのが早い…。
