プログラミング基礎研修でのフルボッコの記録（コードレビュー、PHP）

はじめに

実務未経験でWebエンジニアとして入社してからの2カ月で受けた社内研修についての振り返りです。

同時期に実務未経験でWebエンジニアになって交流している方々（他社の）が、

「実務でのコードレビューでフルボッコになっている」

と言っていて、本人たちは大変そうですが、実装スキルについては爆伸びしていそうで、むしろ羨ましいです。

記事の目的

「他の人が初期にどのような注意を受けているか聞いてみたい」

とも言っていたため、私は自分が書いたコードをレビューしてもらった機会はまだ研修のみですが、どのような点をレビューで教えてもらったかについて、共有しておきたいと思います。

振り返りを通して、私自身が現在忘れてしまわずに身についているか確認していきます。

※ フルボッコというワードは面白くて使っているだけで、フルボッコというか、ただのありがたいコードレビューです。

想定する読者

想定する読者は、過去の自分のような実務未経験の初学者の方です。

どんな社内研修？

社内研修は、Web開発全般についての基礎的な研修で、以下のカリキュラムでした。

1. PHPの基礎（PHP、オブジェクト指向）
2. データベースの基礎（SQL、MySQL）
3. Webアプリケーション基礎（HTML、CSS、CRUD、Cookie、セッション）

これらの内容についての課題を解いていき、 GitLab でマージリクエストして、 社内のエンジニア の方にレビューをいただく、という流れで実施しました。

※ 社内研修コードレビュー記事

例えば、PHPの課題は下記のような内容です。

この商品リストを連想配列として定義し、それぞれについて「〇〇(商品名) は △△(価格)円です。」と出力してください。

配列や条件分岐、クラスメソッドなど基礎文法の理解を問う問題が20問ほどあり、OKもらえるまで修正しました。

目次

社内研修のうち、「PHPの基礎」の課題についてのコードレビューを、一部抜粋して、以下の項目でご紹介します。

1. 読解力
2. 読みやすいコード
3. バグを生まないコード
その他

PHP基礎のコードレビュー

1. 読解力

「出題された要件のとおりに回答するようにしてください」

まずはプログラミングですら無いですが、要件のとおりに実装できていないという指摘を何度かいただきました。要件のとおりに実装するためには、読解力が当然必要になります…。実務では無駄な作業をなるべくしないように、不明点があれば作業前に確認しておくことも必要ですね。

2. 読みやすいコード

「変数名の区別がつきづらいため、どちらかわかるように記載してください」

例えば、「税込価格や税抜価格が出てくる場合、変数名で明確に区別をつけるようにしてください」と、レビューをいただきました。他にもクラス名など、名付けるものには色々と正確に表現するよう指摘をいただきました。

「計算する場所、表示する場所など、固めて書くようにしてください」

オブジェクト指向型は使っていない手続き型で書いた回答（コード）に対してですが、色々とバラバラになっていたため、レビューで指摘をいただきました。

「読む人にとってのわかりやすさを考えましょう」

という指摘を何度もいただきました。

「処理の順序がわかりにくいので、処理手順を、日本語のコメントで記載してください」

コメントは書かない方がいいのかと思っていましたが、実務でも書く、とのことでした。コメントを書くことで、処理の順序がわかりやすくなり、振り返りやすくなりました。
また、コメントを書く場合は、コメント自体が分かりにくくなっていないか、という観点も大事だと思います。

「while文では何もしない場合、それをわかるようにしてください」

while (($data = fgetcsv($handle)) !== false) {
        $count++;
        if ($count === 1) {
            // 1行目（ヘッダ）の場合、合計金額の計算をせずに次の行に進む

「continue;を用いて「1行目の場合は現在の繰り返し処理を終了し、次の繰り返し処理に進む」ように書くと、このwhile文では1行目は何もしないことが分かりやすくなると思います。（書いていないと、他の人が見た時に意図したものなのか、書き忘れなのか分からないため）

一概に「これが正解！」と言うことはできない場合もありますが、重要なのは

他の人が読んだ際にも分かりやすいか、

という観点です。」

と、レビューをいただきました。

3. バグを生まないコード

「型の自動変換を常に意識してください」

// 1. full-bokko.csvファイルを読み込む
if (($handle = fopen("csv/full-bokko.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;

これは常に意識しておくと良いと思います。」

と、レビューをいただきました。

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を使用して修正しました。

「foreachループが終わっても$itemは最後の要素を参照したままなので、unset()しておいてください」

修正後

// 食料品カテゴリーの商品の価格を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()しておいてください」ということになります。

（追記ここまで）

その他

「PHPDocを書いてみましょう」

研修中は以下のような決まりで書きました。現場によって決まってるから、社内での決まりを確認してから書くように、とのことでした。

クラス

/**
 * クラスの概要／要約
 *
 * @auth <開発者名>
 * @version バージョン 日付
 */

変数名

/* @var 型 変数名 */

メソッド名

/**
 * メソッドの概要／要約
 *
 * @param 型 変数名 説明
 * @return 型 説明
 * @throws 例外をスローする場合書く (発生する場合)
 */

「〇〇について説明してみて」

夕会（進捗確認の会）で、「用語や実装を言葉で説明してみて」とよく指導いただきました。例えば、「CSVって何か説明してみて」という感じです。

「ちゃうっ」「何言ってるかわからない」と、ほぼ確実によく言われましたが…笑

人に説明して伝える、という機会は多くあるので、慣れておくように、とのことでした。

自分の言葉で伝えられる、というのが本当に理解した状態だな、と思うので、自分の理解の浅さが歯痒いですが、日々向き合って脳みそに刷り込んでいくしかないかなと思います。

おわりに

「実務でのコードレビューでフルボッコになっている」方々は、最近アウトプットで記事をよく書いていて、記事を通してもフルボッコにより成長されている様子がよくわかります。
私は今はフルボッコな環境ではないですが、負けじとやっていきたいと思います。

ありがとうございました！