Conversation
|
卒業試験の課題実装が完了したので確認お願いします。 質問したいこと
上の２点において今回はeventモデルに判定するためのメソッドを書き、attendances_controllerのcreateアクションでif文の判定がfalseな場合は申し込みができないという方法で実装を行いました。 しかし、この方法だとそれぞれの判定をまとめてtrueかfalseを判断する形になってしまうため失敗した場合のメッセージが１種類しか表示することができずユーザーにとってわかりにくいのではと思いました。 そこで、eventモデルにメソッドを定義するのではなくevent_attendance.rbモデルにバリデーションの形で判定のロジックを記述しそれぞれに異なるエラーメッセージを持たせる方が良いと思ったのですがこの考えが正しいかお聞きしたいです。 |
kerochelo
left a comment
kerochelo
left a comment
There was a problem hiding this comment.
@yusukedayo
そうですね、考え方的にはeventモデルにはeventに関するロジック、event_attendanceモデルに関しては参加周りのロジックをまとめておくのが良さそうですね！
いったんevent_attendanceモデルにロジック組んでみる感じでリファクタしてみましょう！
その他ちょこちょこレビューしてますのでご確認をお願いしますー:pray:
app/models/event.rb
Outdated
| !past? | ||
| end | ||
|
|
||
| def check(user) |
There was a problem hiding this comment.
メソッド名がcheckだけだと、どんなチェックをするのかわかりにくいので、メソッド名をちょっと具体的にしても良さそうですね、そうすることでそのメソッドを使っているviewファイルをみるときにコードが読みやすくなります！
app/models/event.rb
Outdated
| gender_ratio == 'only_man' && user.gender == 'woman' | ||
| end | ||
|
|
||
| def check_participants? |
There was a problem hiding this comment.
こちらは参加可能かどうかの判定ロジックですよね！
であればメソッド名は、can_be_participanted?がいいかなとー(beは短縮してもokです)
ロジック名のイメージは英文を作る感じです！
これを例にすると、eventモデル(目的語)でpartcipantsメソッド(動詞)でuser(主語)って感じです
There was a problem hiding this comment.
ご指摘ありがとうございます！
変数名にわかりやすい名前を付けること意識できていなかったので今後気をつけます！
該当箇所は以下のコミットで修正しました。
9f970dd
app/models/event.rb
Outdated
| end | ||
| end | ||
|
|
||
| def check_all_number |
There was a problem hiding this comment.
ここも出席制限数のチェックなので、わかりやすいメソッド名にしちゃいましょう！
app/views/events/show.html.erb
Outdated
| <% if @event.check_participants? %> | ||
| <%= link_to "参加する", | ||
| event_attendance_url(@event), | ||
| class: 'btn btn-primary', | ||
| method: :post, | ||
| data: { confirm: '申し込みます' } | ||
| %> | ||
| %> | ||
| <% else %> | ||
| 満員です | ||
| <% end %> |
There was a problem hiding this comment.
男性限定や女性限定のイベントの場合の参加ボタンの表示もロジックを作って制御しちゃいましょう！
| </div> | ||
| <div class="mb-3"> | ||
| <%= f.label :gender %> | ||
| <%= f.enum_select :gender, class: 'form-control' %> |
There was a problem hiding this comment.
参考にしたqiita記事を書いた方がそのままgem化して下さってました！
スッキリ書けるので助かってます。
https://qiita.com/tanakaworld/items/ece8718974e14d5a1e55
kerochelo
left a comment
kerochelo
left a comment
There was a problem hiding this comment.
@yusukedayo
いい感じになりましたね！
最後に1点だけコメントしましたのでご確認くださいー:pray:
app/models/event_attendance.rb
Outdated
| def check_set_woman | ||
| return unless Event.find(event_id).gender_ratio == 'only_woman' && User.find(user_id).gender == 'man' | ||
|
|
||
| errors.add(:user_id, 'このもくもく会は女性限定です') | ||
| end | ||
|
|
||
| def check_set_man | ||
| return unless Event.find(event_id).gender_ratio == 'only_man' && User.find(user_id).gender == 'woman' | ||
|
|
||
| errors.add(:user_id, 'このもくもく会は男性限定です') | ||
| end |
There was a problem hiding this comment.
これは1つのメソッドにしちゃってもいいですね！
def check_gender
...
There was a problem hiding this comment.
ご指摘いただいた通りcheck_genderで1つのメソッドに変更しました！
確認お願いします。
ac90fe9
|
@yusukedayo |
2 participants
卒業試験 事業を提案しようの解答を提出しました。
ご確認よろしくお願いします。