こんばんは。
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です。楽しみにしております！