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 さんです。
楽しみですね。