shimamorieiki / remotemaster Goto Github PK

https://dev.classmethod.jp/articles/pull-request-template/

Rollが二つあるけどそれらの違いはなに？
Adminとgeneralにでもするか
管理者しかユーザ登録できないとか当選者を確認できないにする？

Reactでできると嬉しい

[IMAO] timestamps()から生成されるcreated_atやupdated_atはバグ対応などのためにも特にUserには必ずあった方が良いと思います

これまでの運用的に詳細部分がユニークなら許される気がする

パフォーマンスの観点から$winner_nameは一度に取得するのが望ましいです！JOINやEloquentのRelationを使って一度に取得するように変えてみてください！
実際に発行されるSQLは->toSql()やDB::enableQueryLog()を使うことで確認することができます

Laravelで簡単にできるのか？
これってサーバーにデプロイした後の話？

これは難しそう

• 管理者権限と一般ユーザでファイルを分ける
• tokenidを変数で管理する

1とか2のようなマジックナンバーがコード中に出てくるのはあまり良くないです！
島森さんのDB設計だとRoleはプロパティとしてtypeを持っているので、IDよりもこちらを元に管理者かどうかを判断するのが良いと思います！

一例ですが、Roleモデルに以下のようなメソッドを定義して

public function isAdmin(): bool
{
return $this->type === 'ADMIN'; // この値は変えてもOK
}
Controllerやサービス層などでは、このように使うなどの方法が考えられます！

if ($user->role->isAdmin()) {
// 管理者
} else {
// 一般ユーザー
}

@ggg-mzkr ggg-mzkr 3 days ago
上の例のように、$userから紐づいているRoleを直接取得したい場合は、モデルにリレーションメソッドを定義する必要があるので、ご注意ください!

https://readouble.com/laravel/8.x/ja/eloquent-relationships.html

忘れずに実装する

レスポンスを菅井式のstatus,message,responseの形式に変更する

一筆書きのコード について

過去に携わった案件のローカルな表現なので、一般的ではないはず。意味合いは、一つの関数に必要な処理が詰め込まれている状態で、関数やクラスを使って処理を分割していない状態を指します。作り始めた頃は行数が少ないので問題がないのですが、繰り返し仕様追加が行われると、 分岐と繰り返しが複雑に絡み合って修正が困難になります。

複雑度を表す 循環的複雑度 という考え方があるので、参考にしてください。

LotteryControllerにある関数の複雑度を下げるための着目点は下の3点です。

ソースコードにある空行の後にあるインラインコメント
早期リターン
「DBやパラメターからの入力」 、 「加工と処理」 、 「レスポンスやDBへの出力」 の3段階に分離する
1つ目は、処理の塊の境目は空行で区切られているので、区切りごとに処理を分けられます。DBの検索ならモデルのクラスに検索処理を実装します。レスポンスなら レスポンスマクロ が使えます。

2つ目は、途中で処理を継続できないと判断した時点で 処理を終わらせます。具体的には
https://github.com/shimamorieiki/remoteMaster/blob/feature/user/app/Http/Controllers/LotteryController.php#L39
の

if ($min_number == PHP_INT_MAX) {
で return response()->json( ... レスポンスを返します。また、returnするので、 このif文から else を削除することで「該当者がある場合」ということを意識してコードを読む必要がないので、読む人が楽になります。

3つ目は、処理の再利用性や共通化を意識すると、自ずと別れていくと思います。

Admin/tasks GETでタスク一覧を取得
Admin/tasks POSTで新規タスクの追加
Admin/tasks PUTで既存タスクの更新

select ,
count(case tasks.is_positive_check when 1 then 1 else null end) as positive ,
count(case tasks.is_positive_check when 0 then 1 else null end) as negative ,
from
〜以下省略〜

GitHub Actions?

頑張る

マイグレーションファイルで外部キー制約がうまくいかない例があった。
悪い例

$table->foreignid('role_id')->constrained();

良い例

$table->integer('role_id')->unsigned()->constrained();

ここら辺を参考に
https://qiita.com/manbolila/items/c19735438affefbfbe69
https://teratail.com/questions/118706
https://qiita.com/rokumura7/items/867cde3ec42b24825d32
https://qiita.com/yukibe/items/f05bf5e829a9a05616f7
https://www.jaga.biz/laravel/simple-method-for-adding-foreign-key/
https://qiita.com/shosho/items/5ca6bdb880b130260586
https://kapi-travel.com/programing/laravelのマイグレーションで外部キーが通らない場合/
https://qiita.com/yukibe/items/6f7d2c93c8120a41512e

id()かincrements('id')か

・応募したら追加キャンセルはできない？
・応募後にキャンセルし15個を下回ったら削除できない？

本来投票期間はチェック期間の後(同時に行われることはない)なので応募後に「追加、キャンセルはできない」が正しい仕様だと言われた。

[SHOULD]
Controller層に書くコードをもう少し薄くするため、ビジネスロジックをまとめるService層の導入をお勧めします。
https://qiita.com/yukachin0414/items/bb6c94617afd4963ed83

Controller層でリクエストされた値のバリデーション
Service層でビジネスロジックの処理（Modelの呼び出しを含む）を実行し結果をController層に返却
Controller層でレスポンスの値を整形
MVCは意識されていると思うのですが、Controller部分の長めの処理をService層に切り出す方が、より見通しがよくなります。

https://katsusand.dev/posts/laravel-save-data-db/