[CakePHP3]2ヶ月前に終わった自分のOJTのコードレビューをしてみた


こんばんは。
[Fusicアドベントカレンダー](https://qiita.com/advent-calendar/2018/fusic) 17日目の記事です。

## やってみた経緯

今年の6~10月のOJTで、CakePHP3を使って開発したシステムのコードレビューをしてみました。

何を書こうかと過去のアドベントカレンダーを色々と遡っていると、弊社 [@gorogoroyasu](https://qiita.com/gorogoroyasu) が、自分のOJTのコードレビューをしていた[記事](https://qiita.com/gorogoroyasu/items/9c2aa615b79aa64d670a)を見たのがきっかけです。（めっちゃ笑いました）

ちゃんとしたコードレビューはやったことありませんが、面白そうだったのでやってみようと思います。


なお、今回は技術的な話ではなく、ただただ自分の恥ずかしいコードをお見せする回となっていますが、
今後精進していきますので、優しく見ていただけると幸いです。。


いざ、コードレビュー。


## コードレビュー
### index.php

```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](https://github.com/kozo/Partial)とか使った方が良さそう。

---

### Project.php

```php
    public function _getStartDateForView()
    {
        if (!empty($this->start_date)) {
            return $this->start_date->format('Y-m-d');
        } else {
            return '';
        }
    }
```

#### レビュー:
関数名が微妙。 `_getDisplayStartDate()` とかにしたい。
あと、無駄なelseは消してあげたい。

```php
    if(!empty($this->start_date)) {
        return $this->start_date->format('Y-m-d');
    }

    return '';
```

もしくは、いちいちフォーマットするのも面倒なので、 `AppController.php` でコレ書いておくと、View側で良い感じにフォーマットできるのでラク（最近知った）（[参考](https://book.cakephp.org/3.0/ja/core-libraries/time.html#id4))

```php
    public function initialize()
    {
        parent::initialize();

        FrozenTime::setToStringFormat('yyyy-MM-dd HH:mm'); // 不変の DateTime 用
        FrozenDate::setToStringFormat('yyyy-MM-dd'); // 不変の Date 用
    }
```

---

### ProjectsController.php

```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()` って関数名も良くない。（関数名）
今だったらこんな感じで書く。

```php
    $thinkBack = $this->Projects->fetchDisplayThinkback($projectId, $tbdate);
```

[うまくメソッド名を付けるための参考情報](https://qiita.com/KeithYokoma/items/2193cf79ba76563e3db6)


---

### ThinkBacksController.php

```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()` を変数に入れるとかしてほしい。

```php
    $edittedData = $this->PostData->removeEmptyFieldForThinkBack($this->request->getData());    
```


---

### ThinkBack.php

```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;
    }
```
#### レビュー:
無駄な変数宣言と代入。。これで済む。


```php
    if($thumbup['user_id'] == $userId ) {
        return true;
    }
```

---


### ProjectsTable.php

```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)


```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)

```php
    public function saveThinkBackStartDate($thinkBack, $projectId)
    {
        $thinkBacks = TableRegistry::getTableLocator()->get('ThinkBacks');

        // 省略
    }
```

#### レビュー:
テーブルクラスで、自分のテーブルインスタンスを呼び出している。。なんと。。。

しまいにはこの関数、使われてない。


### その参 (ProjectsController.php)

```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](https://qiita.com/Kta-M)です。楽しみにしております！