Posted at
CakePHPDay 5

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

More than 1 year has passed since last update.

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年後の自分の気持を考えなさい。


おまけ


  1. 謎のコメント

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

レビュー:

これ、コメントとして最悪だろう。。。

何をどう書き換えたらいいかわからないし。

こんな書き方するなら放置するな!


  1. マジックナンバー

if ($currentPhaseId == $nextPhaseId) {

$applicant->pass = 1;
} else {
$applicant->pass = null;
}

レビュー:

1ってなんだよ!!!怒

あと、数値 or null ってだめだろ。。。


  1. 不要な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 さんです。

楽しみですね。