こんばんは。
Fusicアドベントカレンダー 17日目の記事です。
やってみた経緯
今年の6~10月のOJTで、CakePHP3を使って開発したシステムのコードレビューをしてみました。
何を書こうかと過去のアドベントカレンダーを色々と遡っていると、弊社 @gorogoroyasu が、自分のOJTのコードレビューをしていた記事を見たのがきっかけです。（めっちゃ笑いました）
ちゃんとしたコードレビューはやったことありませんが、面白そうだったのでやってみようと思います。
なお、今回は技術的な話ではなく、ただただ自分の恥ずかしいコードをお見せする回となっていますが、
今後精進していきますので、優しく見ていただけると幸いです。。
いざ、コードレビュー。
コードレビュー
index.php
<div class="card fs-08 mt-3">
<div class="card-header view-table-header">
<h3 class="card-title">プロジェクト一覧</h3>
</div>
<?php echo $this->element('search_project') ?>
</div>
レビュー:
ここでしか使われないプロジェクトの検索窓をelementに記述している。
elementは、複数のテンプレートの共通部分に使うので、Partialとか使った方が良さそう。
Project.php
public function _getStartDateForView()
{
if (!empty($this->start_date)) {
return $this->start_date->format('Y-m-d');
} else {
return '';
}
}
レビュー:
関数名が微妙。 _getDisplayStartDate() とかにしたい。
あと、無駄なelseは消してあげたい。
if(!empty($this->start_date)) {
return $this->start_date->format('Y-m-d');
}
return '';
もしくは、いちいちフォーマットするのも面倒なので、 AppController.php でコレ書いておくと、View側で良い感じにフォーマットできるのでラク（最近知った）（参考)
public function initialize()
{
parent::initialize();
FrozenTime::setToStringFormat('yyyy-MM-dd HH:mm'); // 不変の DateTime 用
FrozenDate::setToStringFormat('yyyy-MM-dd'); // 不変の Date 用
}
ProjectsController.php
public function view($id = null)
{
// 省略
// $tbdateの有無で処理を分ける
if(empty($tbdate)) { // 最初のアクセス時
// 最新の振返りを最初に表示
$thinkBack = $this->Projects->getInitialThinkback($projectId);
} else {
// 表示する振り返りを取得
$thinkBack = $this->Projects->getDisplayThinkback($projectId, $tbdate);
}
// 省略
}
レビュー:
if文要らないなあ。。
getInitialThinkback()、getDisplayThinkback() って関数名も良くない。（関数名）
今だったらこんな感じで書く。
$thinkBack = $this->Projects->fetchDisplayThinkback($projectId, $tbdate);
ThinkBacksController.php
public function edit($id = null)
{
$this->loadComponent('PostData');
if ($this->request->is(['patch', 'post', 'put'])) {
$this->request = $this->PostData->removeEmptyFieldForThinkBack($this->request);
$thinkBack = $this->ThinkBacks->patchEntity($thinkBack, $this->request->getData(), [
'validate' => 'editThinkBack'
]);
if ($this->ThinkBacks->save($thinkBack)) {
// 省略
}
}
うおお、 $this->request をそんまま書き換えとる。。
必要があったとしても、 $this->request->getData() を変数に入れるとかしてほしい。
$edittedData = $this->PostData->removeEmptyFieldForThinkBack($this->request->getData());
ThinkBack.php
/**
* 自分自身が振返りに対してイイねをしているか判断し、しているならtrue、してないならfalseを返す
*/
public function existThumbup($userId)
{
// thumbupしてあるuser_idをひとつずつ見ていく
foreach($this->think_back_thumb_ups as $thumbup) {
$thumbupUserId = $thumbup['user_id'];
// 自分のuserIdが存在している場合、1を返す
if($thumbupUserId == $userId) {
return true;
}
}
// 自分のuserIdが存在しなかったときの処理
return false;
}
レビュー:
無駄な変数宣言と代入。。これで済む。
if($thumbup['user_id'] == $userId ) {
return true;
}
ProjectsTable.php
public function getDisplayThinkback($id, $displayThinkbackDate)
{
$displayThinkback = $this->ThinkBacks->find()
->where(['ThinkBacks.project_id' => $id])
->contain([
'Projects',
'KptKeeps',
'KptProblems',
'KptTries',
'KptToDos',
'KptActions',
'WorkTimes',
'ThinkBackThumbUps'
])
->where(['ThinkBacks.finish_date' => $displayThinkbackDate]) // $this->request->getQuery('tbdate') = 'Y-m-d'
->first();
return $displayThinkback;
}
レビュー:
// $this->request->getQuery('tbdate') = 'Y-m-d'
意味不明なコメント。。。何を伝えたかったのか当時の自分に問いたい。
それと、結局 thinkBack のエンティティを返すなら ThinkBackTable.php に書こう。（同じようなのが他に３つあった)
今では許せないシリーズ
その壱 (ThinkBacksController.php)
public function index()
{
// 省略
$this->set('thinkBacksList', $thinkBacksList);
$this->set('startIndex', $startIndex);
$this->set('countOfLoadedTimelineItems', ThinkBacks::countOfLoadedTimelineItems);
$this->set('isFinshed', $isFinshed);
}
たのむ、$this->set(compact('thinkBackList', ...)); でまとめてくれぇ。。。
その弐 (ThinkBacksTable.php)
public function saveThinkBackStartDate($thinkBack, $projectId)
{
$thinkBacks = TableRegistry::getTableLocator()->get('ThinkBacks');
// 省略
}
レビュー:
テーブルクラスで、自分のテーブルインスタンスを呼び出している。。なんと。。。
しまいにはこの関数、使われてない。
その参 (ProjectsController.php)
public function add($projectId = null)
{
$projects = TableRegistry::getTableLocator()->get('Projects');
$project = $projects->find()
->where(['id' => $projectId])
->firstOrFail();
//
// 省略
//
$projects = TableRegistry::getTableLocator()->get('Projects');
$project = $projects->find()
->where(['id' => $projectId])
->firstOrFail();
どうしてこうなった。。
2度も発行されるクエリの気持ちになってほしい。
しかも、お前は $this->Projects というのを知らんのか。
感想
昔の自分のコードレビュー、やってると楽しくなってきます。
他にも色々と突っ込みたいコードはあったんですが、分量の関係で(ry
今になってOJTのコードを見ると、「今だったらこう書くよなあ」とか「この書き方合ってんだっけ？」とか色々あります。
それから調べるきっかけができるので、勉強にもなりオススメです。
あと、無性にリファクタリングしたくなります。 ←重要
OJT終わりの方は、できるようになった部分が見えると思うので、ぜひやってみてください。
これ間違ってない？などありましたら、ご教授頂けると幸いです。。
さて、明日は@Kta-Mです。楽しみにしております！