未経験半年で作成したWebアプリを、実務半年経ったのでリファクタリングしてみた

皆さんは最初に書いたコード覚えていますでしょうか。

目的

18年の12月にPHPを勉強し始め、19年の3月頃からLaravelを勉強し始めました。
その後、19年の5月20日から6月15日までLaravelを使ったオリジナルアプリを作りました。

当時Twitterで行われている100DaysOfCodeというものを行っており
活動時間とかわかればいいなーと思い作りました。

作った当初は良いと思っていたのですが、エンジニアとして現場に出て半年、久々に見るとひどいなと思い
何がだめだったのか、供養がてらリファクタリングしてみようとやってみました。（※厳密にはリファクタリングではありません->若干挙動に修正が入っているため）

なお、ユーザーが全く居ないのでデータ移行は考慮してないです

もっとこう出来る等ありましたら、ぜひ教えていただけますと幸いです。

作ったもの

動作リンクはこちら

Topページ

• Twitterで登録・ログインを押すと、ソーシャルログインが行われマイページへリダイレクトします

マイページ

• 画面下部の入力欄に継続したい活動の名称を登録できます
• 登録した活動が一覧表示されます

投稿ページ

• 活動の内容や時間などを入力し、投稿するとツイートされ、活動時間、活動日数、継続日数が記録されます
• 画像には映っていませんが、投稿を削除でき、削除するとツイートも消えます。

簡単にまとめると

• トップページから、ログイン・登録をおすと、Twitter認証が行われる
• ログイン後、マイページへ移動でき、そこで活動を作成
• 作成した活動の進捗などをTwitterにつぶやく
• 合計活動時間や継続日数などが記録されていく

というものです。

環境

当時のデプロイした環境は下記になります。

PHP 7.2
FW Laravel 5.5
MySQL 
ConohaVPS

リファクタリング

1.DB関係

ER図はこんな感じです。

マイグレーションファイルは下記のようになっています。

Schema::create('users', function (Blueprint $table) {
    $table->increments('id');
    $table->string('twitter_id')->unique();
    $table->string('twitter_name')->nullable();
    $table->string('twitter_nickname')->nullable();
    $table->string('twitter_avatar')->nullable();
    $table->string('twitter_oauth_token')->nullable();
    $table->string('twitter_oauth_token_secret')->nullable();
    $table->rememberToken();
    $table->timestamps();
    $table->softDeletes();
});

Schema::table('users', function (Blueprint $table) {
    $table->string('twitter_id', 191)->change();
});

Schema::create('activities', function (Blueprint $table) {
    $table->increments('id');
    $table->integer('task_id')->nullable()->default(0);
    $table->integer('user_id');
    $table->string('name')->nullable()->default(null);
    $table->string('hour')->nullable()->default(0);
    $table->integer('continuation_days')->nullable()->default(0);
    $table->timestamps();
    $table->softDeletes();
});

Schema::table('activities', function (Blueprint $table) {
    $table->integer('days_of_activity')->nullable()->default(0);
});

Schema::create('tweets', function (Blueprint $table) {
    $table->increments('id');
    $table->string('user_id')->nullable();
    $table->integer('activity_id');
    $table->string('tweet_id');
    $table->text('body')->nullable();
    $table->string('hour')->nullable()->default(0);
    $table->timestamps();
    $table->softDeletes();
});

何がだめだったか

まず、ほんとうにそのカラムが必要なのか考えていませんでした
行き当たりばったりで作った記憶があります。
正規化もよく分かっていませんでした。

命名も
半年ぶりに見るとなんのカラムを表しているのかわかりませんでした。
tweetsテーブルがtweet_idを持っていたり、task_idという謎のカラムもあり自分で作ったくせに悩みました。

わかりやすい命名とコメントをつけるべきでした。
また下記のように、カラムにもコメントをつけるべきでした。

$table->integer('hoge')->comment('ほげ');

次に型です。
tweetsテーブルのもつuser_idやhourがなぜかstring(varchar)だったりします。

インデックスと制約についてですが、特に考量していませんでした。
存在は知っていましたが付け方を知らなかった気がします。

修正後

tweetsテーブルをpostsテーブルに変更しました。

また、活動内容を投稿するたびにactivitiesの持っていた合計時間格納用カラムに加算してましたが、postsからSQLで集計するようにしたため削除。
継続日数もロジックでなんとかなりそうだったので削除しました。

usersはidをtwitterのユーザーidにし、他のカラム名も変更しています。

その他、外部キーも設定し、その他検索で使うと思われるものにindexを貼っています(この小規模ので必要かはわかりませんが)。

2.ルーティング

web.php

Route::get('/', function () {
    return view('top', ['user' => Auth::user()]);
})->name('top');

Route::prefix('auth/twitter')->group(function () {
    // ログインURL
    Route::get('/', 'Auth\LoginController@redirectToProvider')->name('login');
    // コールバックURL
    Route::get('/callback', 'Auth\LoginController@handleProviderCallback');
    // ログアウトURL
    Route::post('logout', 'Auth\LoginController@logout')->name('logout');
});
Route::group(['middleware' => ['auth', 'user.name'], 'prefix' => 'activity'], function () {
    Route::get('{user_name}/', 'ActivityController@index')->name('activity.index');
    Route::post('{user_name}/', 'ActivityController@store')->name('activity.store');
    Route::get('{user_name}/{activity}', 'ActivityController@show')->name('activity.show');
    Route::patch('{user_name}/{activity}', 'ActivityController@update')->name('activity.tweet');
    Route::delete('{user_name}/{activity}', 'ActivityController@destroy')->name('activity.delete');
    Route::delete('{user_name}/{activity}/{id}', 'ActivityController@deleteTweet')->name('tweet.delete');
});

図で表すとこのようになります。

何がだめだったか

もっとまとめられました。

修正後

web.php

<?php

Route::view('/', 'top')->name('top');

Route::group(['prefix' => 'auth/twitter'], function () {
    // ログインURL
    Route::get('/', 'Auth\LoginController@redirectToProvider')->name('login');
    // コールバックURL
    Route::get('/callback', 'Auth\LoginController@handleProviderCallback');
    // ログアウトURL
    Route::post('logout', 'Auth\LoginController@logout')->name('logout');
});
Route::group(['prefix' => '{user_name}', 'middleware' => ['auth', 'user.name']], function () {
    Route::resource('activity', 'ActivityController')->only(['index', 'store', 'show', 'update', 'destroy']);
    Route::resource('activity/{activity}/post', 'PostController')->only(['store', 'destroy']);
    Route::get('activity/{activity}/post/latest', 'PostController@fetchLatest')->name('post.latest');
});

topページはRoute::viewというものがあるので、それを用います。
また、Route::prefixをgroupに変更
resourceでまとめられるものをまとめています。

投稿に関するものをActivityControllerで処理していたので(継続日数や活動時間を投稿のたびに、活動へアップデートしていた)
PostControllerへ切り離しました。

topで使用されているAuth::user()ですが、他のviewで共通して使用していたのでViewComposerを用いて自動的に必要なViewに渡すようにしています。

3.Controller

Controller共通

Controller内でバリデーションや登録等すべての処理をしていましたが、

• FormRequestでバリデーション
• コンストラクタインジェクションした、サービスクラスにバリデーション済みデータを渡す
• 処理した結果を受け取る

上記のように変更しました。
Modelはリレーション、アクセサ、スコープを書く用にしています。

ActivityController.php

    /**
     * 活動の保存
     * @param  ActivityStoreRequest $request
     * @return \Illuminate\Http\RedirectResponse
     */
    public function store(ActivityStoreRequest $request)
    {
        $result = DB::transaction(function () use ($request) {
            return $this->activity_service->createActivity($request->validated());
        });
        return redirect()->back()->with($result ? 'success' : 'error', $result ? '新規追加しました' : '追加に失敗しました');
    }

Login処理

下記はTwitterの認証画面で承認された際のリダイレクトを処理するauth/twitter/callbackの部分です。

Auth/LoginController.php

    /**
     * ユーザーのアクセストークンを取得する
     *
     * @return \Illuminate\Http\Response
     */
    public function handleProviderCallback()
    {

        try {

            $twitter_user = Socialite::driver('twitter')->user();  
            //アクセストークン取得
            $token = $twitter_user->token;
            $token_secret = $twitter_user->tokenSecret;
            if (!$twitter_user || !$token || !$token_secret)
                throw new \RuntimeException('ユーザー取得の失敗');


            //ユーザーの取得または生成
            $user = User::firstOrCreate(['twitter_id' => $twitter_user->id]);
            //最新状態に更新
            $update_result = $user->update(
                [
                    'twitter_name' => $twitter_user->name,
                    'twitter_nickname' => $twitter_user->nickname,
                    'twitter_avatar' => $twitter_user->user['profile_image_url_https'],
                    'twitter_oauth_token' => $token,
                    'twitter_oauth_token_secret' => $token_secret
                ]
            );

            if (!$user || !$update_result)
                throw new \RuntimeException('ユーザー情報の生成または更新の失敗');

            Auth::login($user, true);
            return redirect()->route('activity.index', $twitter_user->nickname)->with('success', 'ログインしました');

        } catch (Exception $e) {
            Log::error($e->getMessage());
            return redirect()->route('top')->with('error', 'Twitterアカウント取得に失敗しました');
        }

        return redirect()->route('top')->with('erorr', 'エラーが発生しました。再度お試しください');
    }

何がだめだったか

まず、謎に通らない箇所にリダイレクト処理があります。
作成・更新がありますがトランザクションがありません。
また、取得できなかったら、作成、その後更新をしていますが、updateOrCreateで済みます。

Serviceに切り出したりしたらもっとスッキリできそうな気がするのですがこれで進みました。

Auth/LoginController.php

    /**
     * ユーザーのアクセストークンを取得、ログインをする
     *
     * @return \Illuminate\Http\Response
     */
    public function handleProviderCallback()
    {
        // twitterからアカウント情報取得
        try {
            $account_info = Socialite::driver('twitter')->user();
        } catch (\Exception $e) {
            Log::error($e->getMessage());
            return redirect()->route('top')->with('error', 'Twitterアカウント取得に失敗しました');
        }
        $auth_user = DB::transaction(function () use ($account_info) {
            $user = User::updateOrCreate(['id' => $account_info->id], [
                'name' => $account_info->name,
                'nickname' => $account_info->nickname,
                'avatar' => $account_info->avatar,
                'token' => $account_info->token,
                'token_secret' => $account_info->tokenSecret
            ]);
            Auth::login($user, true);
            return $user;
        });

        return redirect()->route('activity.index', $auth_user->nickname)->with('success', 'ログインしました');
    }

4.フロント周り

なにがだめだったか

もともとこのアプリはHTML、CSSは無料テンプレートのを使っていたのですが、
不必要なライブラリなどたくさん読み込んでいました。
しかも、minify化(コードの圧縮化)がされていないものでした。

また、jQueryを使っていたのですが、
Bladeにベタ書きしており、更にBlade自体コンポーネントへ分割していなかったので非常に見づらくなっていました。

ライブラリ類はnpmからインストールし、
Laravel Mixを用いてjsのコードを圧縮しました。

Bladeも@pushや@include、@componentを使い切りだせそうなものは切り出しました

5.Log

これは未だにどうするのが良いのか、はっきりと分かってはいません
もともとは、エラー箇所にLogを書き込む処理おいているだけでした。

今回は
Eloquentは作成や更新などイベントが起こるとそれをフックに、そのタイミングで処理ができるので、そこにLog処理を置きました。

下記のようにすると、Userが作成されたタイミングでLogが書かれます

php artisan make:observer UserObserver --model=User

UserObserver.php

<?php

namespace App\Observers;

use App\User;
use Illuminate\Support\Facades\Log;

class UserObserver
{
    /**
     * Handle the user "created" event.
     *
     * @param  \App\User  $user
     * @return void
     */
    public function created(User $user)
    {
        Log::info('ユーザーが新規作成されました', ['user' => $user->nickname]);
    }
}

6.最適化

一番最初、完成した後やったことは
.envのAPP_ENVをproductionにし、APP_DEBUGをfalseにしただけでした。

なにがだめだったか

公式にかかれている最適化処理があるのに関わらず、行っていませんでした。

オートローダー最適化

composer install --optimize-autoloader --no-dev

設定ローディングの最適化

php artisan config:cache

ルートロードの最適化

php artisan route:cache

その他の最適化として、Opcacheの有効化を行いました。

OPcacheとは

スクリプト言語は毎回呼び出される度にプログラムの解析・解釈が行われています。 コンパイル型の言語に比べてスクリプト言語は、「呼び出し毎にプログラムを解析する」点にオーバーヘッドがあると言えます。

スクリプト言語においても、ソースコードが変更されていないのであればコンピュータが解釈をし直す必要がないのではないかと考えられます。 よって、コンピュータが解析済みの「バイトコード」をキャッシュして再利用すればよく、そういう機能をアクセラレータとよびます。 PHPのアクセラレータ機能を利用することでPHPを解釈する段取りをスキップすることができ、処理が高速になる

他に、HTMLの圧縮を行いました。
htmlmin/htmlminというパッケージを使用し、Bladeからコンパイルされたファイルを圧縮しています。
自身で作ったディレクティブ(@hogeのようなもの)があると、うまく使えません。

その他個人的な

ツイートの文字数カウント

今まで、jsでkeyUpのたびにカウントする、Laravel側はmax:140のバリデーションというガバルールだったのを
twitter-text-jsを使って、Twitterと同様のカウントにしました。
Laravelも同様にパッケージを使いTwitterと同様のルールでバリデーションできるようにしました。
js:詳しくはこちら
Laravel:詳しくはこちら

継続日数のカウント

今まで投稿するたびに、合計時間も活動日数も継続日数もいちいち計算して活動(activities)のカラムへ保存していました。
投稿から取得して計算するように変更したところ、コード量がごっそり減りました。

Before

After

おわりに

半年前のコードが完全に他人のもので、想像以上に時間がかかりました。
頭の中で考えてとりあえず作るということのデメリットが享受できました。
今後は事前の設計をしっかりして、物を作りたいです。

また半年したらこのコードもだめだなと思えるような成長を遂げていたいものです。