[CakePHP3 Advent Calendar]CakePHP3で書いた自分のコードレビューしてみた。 #PHP

CakePHP Advent Calendar 5日目の記事です。
昨日は、@o0h さんの [CakePHP3]現場で使えるCollectionクラスの15選でした。
自分は、 array_filter とか array_map とか、PHP の組み込み関数をよく利用していたので、
参考にさせて頂きます。

自分のコードレビューしてみた。

今日は、あまり技術の的な話ではないです。
ご了承ください。

単純に、自分が昨年書いたコードを公開レビューしていこうと思います。
自分自身の黒歴史を晒しつつ、反省しようと思います。
お恥ずかしいコードをお見せしますが、
一年前の僕を許してあげてください。。。

ちなみに、対象のコードは、自分が入社して半年ぐらいで書いた、
OJT のコードです。
https://fusic.co.jp/doings/118
で紹介されているので、興味があったら御覧ください。

では、はじめます。

config/bootstrap.php

// 現職・離職
define('NOT_WORKING', 0);
define('WORKING', 1);
// ステータス関連
define('BEFORE_CONTACT', 1);
define('NEW_ARRIVAL', 4);

コメント:
bootstrap に define。。。
せめて、StatusesDefine.php みたいなファイルにクラスを作って、


class StatusesDefine 
{
    const NOT_WORKING = 0
}

とかにして、
外部からは、
StatusesDefine::NOT_WORKING
と呼ぶべきだろう。

DataCasting.php

その1

public static function calcCurrentAge($birthday, $appliedDate)
{
    $birthday = new FrozenTime($birthday);
    $appliedDate = new FrozenTime($appliedDate);
    $age = $appliedDate->format('Ymd') - $birthday->format('Ymd');
    return (int)floor($age/10000);
}

レビュー:
DataCasting.php は、自分で作ったクラスらしい。
このコード、何をやってるのかパット見で分からない。
まず言いたいのが、FrozenTime ではなくて、 FrozenDate を使うべきだろう。
お前は、秒単位で誕生日を求めたいのか？
ちなみに、 (int)floor($age/10000) は、割とよく使われるらしい。

[参考]
http://sonaiyuutemo.hatenablog.jp/entry/2017/06/20/185329
https://qiita.com/arthur87/items/aa3355f218d4c9a90436

その2


    public static function isCorrectFotmatForDate($string)
    {
        $checkedString = DataCasting::removeHyphen($string);
        if (!is_numeric($checkedString)) {
            return false;
        }
        if (strlen($checkedString) != EIGHT_DIGITS) {
            return false;
        }
        return true;
    }

レビュー:
なぜ、DataCasting にこのコードがあるか分からない。
少なくとも、Validation でやるべきだろう。
たぶん、彼は ModelLessForm とか知らなかったに違いない。
あと、コメント書け。

その3

    public static function returnDateTime($string)
    {
        if (!Validation::date($string)) {
            return false;
        }
        return new FrozenTime($string);
    }

レビュー:
なるほど。やりたいことは分かる。
でも、なぜここに有るかについては不明。
あと、 isCorrectFotmatForDate 作ったなら使え！！
使わないなら作るな！！！

ApplicantsProcessingPhasesTable.php

public function loggingProcessingPhases ($id = null, $processingPhase = null)
{
    //
    // 省略
    //
    if (empty($entity->errors()) && $deleteEntities) {
        foreach($deleteEntities as $deleteEntity) {
            $data = $this->get($deleteEntity->id);
            $this->delete($data);
        }
    }
    return $this->save($entity, ['atomic' => false]);
}

レビュー:
これ、transaction かけたいんだろうけど、
delete には ['atomic'=>false] がないし、
そもそも、ConnectionManager を呼んんでない。
delete でこけたらどうするつもりなんだろう？
せめて、

if (!$this->delete($data, ['atomic' => false])) {
    return false;
}

とすべきだろう。

ProcessingPhasesTable.php

/**
* ビット演算
* [returnBinaryCastedValue description]
* 今回の場合、id が1から始まるので、id の開始を 0 からに修正するためのコード
* 応募者の応募種別IDを利用して、応募者の権限を出力する。
* @param  [type] $typeId 応募者の種別ID 又は 職種ID
* @example if id == 1 =>　1を左に (id - 1 ) = 0　ビットずらす。　つまり、0b0001;
* @example if id == 4 => 1 を左に (id - 1 ) = 3 ビットずらす。　つまり、0b1000;
* @return [type]         [description]
*/
private function returnBinaryCastedValue($typeId = null)
{
    return 1 << ($typeId - 1);
}
/**
* ビット演算
* [checkIsCorrectType description]
* 応募者の種別ID を利用して、その応募者が権限を持っているかどうかを判定する。
* @param  [binary] $binary   チェックされる値 例) 0b0100;
* @param  [type] $typeBinary チェックする値。 例) 0b1100;
* 上記の例の場合、 true が返される。
* @return [type] boolean
*/
private function checkIsCorrectType($binary = null , $typeBinary = null){
    return ($binary & $typeBinary) ? true : false;
}

レビュー:
丁寧にコメントが！！！
と思いきや、ビット演算してやがる。。。
こんなのどうやってメンテするんだ馬鹿野郎(泣)
ここの仕様は確かに若干複雑だが、 entity に書けばまあなんとかなる感じだろうと思う。
当時の俺は、 bit 演算使うのがかっこいいと思ってたのだろう。
1年後の自分の気持を考えなさい。

おまけ

謎のコメント

// TODO もう少しまともな処理に置き換える。

レビュー:
これ、コメントとして最悪だろう。。。
何をどう書き換えたらいいかわからないし。
こんな書き方するなら放置するな！

マジックナンバー

if ($currentPhaseId == $nextPhaseId) {
    $applicant->pass = 1;
} else {
    $applicant->pass = null;
}

レビュー:
1ってなんだよ！！！怒
あと、数値 or null ってだめだろ。。。

不要なtoArray()

    $query = $this->NewGradInformations->find();
    $entity = $query->toArray();
    $sender = [];
    foreach ($entity as $value) {
        if (!in_array($value[$type], $sender)){
            array_push($sender, $value[$type]);
        }
    }

レビュー:
toArray() いらなくね？

あと、Collection 使えばもっといい感じに書けそう。。。

あとがき

という感じで、自分が1年前に書いたコードレビューしてみました。
なんか、けっこう面白かったです。

ほとんどコード載っていないですが、
もし何か間違いに気づいた方はご一報ください。
例えば、今の自分だったら、StatusesDefine.php 作るけど、
これがCakePHP として正しいかは自信ないです。

感想

たまには、30分ぐらいで昔の自分のコードをレビューしてみるのも面白いなと思いました。
時間を見つけてリファクタします(bit演算以外)
今だったら、来年の自分を唸らせるコードが書ける気がする！

明日は、 @mosa7 さんです。
楽しみですね。