今まで触れてきたコードを少しだけ改善する

はじめに

この記事は、 CAMエンジニア Advent Calendar 2019 日目の記事です。

記事一覧はこちらから↓
https://qiita.com/advent-calendar/2019/cam-inc

今年も終わりに近づいてきました。
記事を書くのは初めてなので、何を書くべきか考えましたがなかなか思い浮かばす。どうしようかな？ん〜。
ま〜いつもやっている仕事の内容を書こうかなっと思いました。
現在私が携わっているプロジェクトは結構前からあるのですが、何年も前から存在しているコードだと人が入れ替わったり、時間的余裕などでその場しのぎのコードを実装してしまうことがよくあると思います。しょうがないのですがそれによって技術負債が溜まり、後にそのプロジェクトにアサインされた人が怒り狂うことは可能性として低くはないでしょう。
そこで今回の本題。会社に入ってプログラミング経験1年未満の私が、現プロジェクトのコードを改善し、ちょっとだけ読みやすいコードにしていこうと思います。
コードはPHP7、CakePHP2, controller内のお話です。
あと、ちょー初心者向けなので、当たり前ですが、大事なこと書いていこうと思います。
改善前のコードはもちろんプロジェクトコードそのままではありません。

コード改善例1

では、実際にあった編集画面のコードです。

public function edit ($id) {
    $data = $this->model->find($id);
    // 編集権限があるか
    if ($user === $member) {
        if ($this->request->is('POST')) {
            $inputData = $this->request->data;
            $this->Model->set($inputData);
            if ($this->Model->validates()) {
                $this->Model->save();
            } else {
            // バリデーションNGの場合の処理
            }
        } else {
            if (!empty($data)) {
                // データベースから取得した値
                $this->request->data = $data;
            } else {
                // データが空っぽ
            }
        }
    } else {
        // ログイン画面に飛ばす
    }
return;
}

はい、上記が改善前のコードです。どうでしょうか？結構簡潔なコードですが、よくない点がありますね。
簡潔なコードすぎて、そんなにいっぱい改善点はありませんが、パッと見思うのが「if文のネストが深い!」だと思います。
でも、結構こんなコードありますよ!
では、初心者なりの改善を下記に記します。

変更後

public function edit ($id = null)
{
    // 編集権限があるか
    if ($user !== $member) {
        // ログイン画面に飛ばす
    }

    if (! $id) {
        // idがなければリダイレクト
    }

    if ($this->request->is('GET')) {
        $data = $this->Model->find($id);
        if (empty($data)) {
            // データがなければリダイレクト
        }
        return;
    }

    $inputData = $this->request->data;
    $this->Model->set($inputData);
    if (! $this->Model->validates()) {
        // バリデーションNGの場合の処理
        return;
    }
    // 保存処理
    $this->Model->save();
}

こんな感じですかね。
改善点1
まず、ネストを全体的に浅くしました。初心者あるあるで、確認のコードが増えるとif elseで結構ネストが深くなってしまいます。そうならないために処理を個別化し、期待した処理でなかった場合はその場でredirectやreturnで終わらせることがポイントです。

改善点2
無駄なクエリを無くしました。改善前は、getでもpostでもDBのデータを持ってきていました。でも実際にDBのデータが必要なのはgetで既存のデータをformに入れる時だけ。今回は管理画面なのでDBに対する負荷はほぼ無いですが、多くのユーザが利用するサービスの場合はクエリの数を減らすことが重要です。

改善点3
カッコの位置や不要なreturn、ここは好みかもしれませんが、PSRに則って実装することでコードの統一感も出ますし、規約に厳しい人に嫌味を言われる確率も下がります!
アクションの最後のreturnは必要なのでしょうか？「必要です！」と言われればそうなのか〜と思ってしまいますが、、、

あと、「会員確認は親クラスやbeforefilerで確認しろ！」などの意見があると思いますが、今回はこのアクション内で簡潔に改善しようというのが目的です。優しい意見は受け付けております(๑╹ω╹๑ )

コード改善例2

もう一つだけ紹介させてください
改善前

public function change($tmp)
{
    if ($tmp === 'AAA') {
        $name = 'aaa',
    } elseif ($tmp === 'BBB') {
        $name = 'bbb'
    } elseif ($tmp === 'CCC') {
        $name = 'ccc'
    } elseif ($tmp === 'DDD') {
        $name = 'ddd'
    } elseif ($tmp === 'EEE') {
        $name = 'eee'
    } else {
        $name = false;
    }
    return;
}

例２つ目でございます。このコードは引数の文字列によって値を返すものです。どうでしょうか？別に問題無い感じでしょうか？
例えば、これが、DBに入れるほどでもないデータだが、50ぐらい増えた場合どうでしょう？もし、最後のelseifだった場合は無駄な処理が、走りますね。可読性をあげようとしてswitch文にしても性能は同じぐらいだと思います。
初心者的に改善するとしたらこうなります。

改善後

public function change($tmp)
{
    $array = [
        'AAA' => 'aaa',
        'BBB' => 'bbb',
        'CCC' => 'ccc',
        'DDD' => 'ddd',
        'EEE' => 'eee',
    ];
    if (! array_key_exists($tmp, $array)) return false;

    $name = $array[$tmp];
    return $name;
}

こんな感じに私は改善しました。本当はConfigなどに定義しておくのが良いと思うのですが、今回はcontrollerに書かせていただきます。あと、文字列とbool値を返しているのもご了承ください。PHPは型にゆるい言語なので
( ；∀；)
今回はnameだけだったので、順序配列ですが、

$array = ['AAA' => ['name' => 'aaa']]

とすれば、name以外もあとで付け足すことができます。

終わりに

繰り返しになりますが、今回初めて記事を書かせていただきました。言葉が稚拙なところがあるかもしれませんが、最後まで読んでいただきありがとうございました。コード手書きなので、間違っているところがあったら教えていただけると幸いです。テキストエディタって便利ですね、キータでコード手書きはきついです。(コピペの方法がわかりませんでした)