Kumo raku step12 add end date #1459
Conversation
KumoRaku
requested review from
zackey-y,
kenkaton and
rr-woochul-shin
and removed request for
zackey-y and
kenkaton
| @tasks = case params[:sort] | ||
| when 'latest' | ||
| Task.latest | ||
| when 'expiring' | ||
| Task.expiring |
There was a problem hiding this comment.
case whenを使って判定すると今後パラメータの値が増えた時にコントローラーが膨大になりそうです。極端にいうと方向asc desc両方対応 + クリアまで考えた時に5つの条件ができると思います。
modelに判定を書くとコントローラーを軽くかけるのでこういう書き方はどうですか？
index
def index
@tasks = Task.expiring(params[:sort]).latest
endmyapp/app/models/task.rb
scope :latest, -> { order(created_at: :desc) }
scope :expiring, ->(sort) { order(end_date: :asc) if sort.eql?('expiring') }もっというとselect boxなどを作ってasc descの部分をパラメータとかで取得するようにし、
end_date前後をボタン繰り返し選択で変えっるようにするともっといいと思います
There was a problem hiding this comment.
ご指摘ありがとうございます。
確かにそうです、
今後時間の余裕があればこういうロジックをusecase層で実装したいなぁと思ってますが、
今回はmodelでsearchメソットを追加してそこで実装する。
Step13で検索機能が入ってるのでそれも兼用できると思います。
いかがでしょうか？
myapp/app/models/task.rb
Outdated
| scope :latest, -> { order(created_at: :desc) } | ||
| scope :expiring, -> { order(end_date: :asc) } |
myapp/spec/system/tasks_spec.rb
Outdated
| context 'when there are tasks,' do | ||
| let!(:task1) { FactoryBot.create(:task) } | ||
| let!(:task2) { FactoryBot.create(:task, title: 'Spec2', description: 'test2') } | ||
| let!(:task2) { FactoryBot.create(:task, title: 'Spec2', description: 'test2', end_date: Time.new(2023, 11, 1, 10, 30)) } |
There was a problem hiding this comment.
let!にする必要はないと思いました。
let!は遅延評価されてパフォーマンス的に良くないと思います。
あとlet!を使う必要があるようなデータを作成するようにも見えませんでした。
before内でまとめましょうか
before do
FactoryBot.create(:task)
FactoryBot.create(:task, title: 'Spec2', description: 'test2', end_date: Time.new(2023, 11, 1, 10, 30))
endThere was a problem hiding this comment.
参考議論なども併せて確認してください
rubocop/rspec-style-guide#55 (comment)
|
@sourcewc さん |
myapp/app/models/task.rb
Outdated
| def self.search(conditions) | ||
| order("#{conditions['sort_column']} #{conditions['sort_direction']}") | ||
| end |
There was a problem hiding this comment.
こういう書き方にするとユーザが任意の操作でデータベースの並び順を帰れます。
あと間違ったパラメータを渡すとアプリケーションエラーが発生するので直しましょう、、
Search用で使用するメソッドではないと思います
There was a problem hiding this comment.
こういう書き方にするとユーザが任意の操作でデータベースの並び順を帰れます。
すみません、ここは理解できなかった。もう詳しく説明お願いいただければありがたいです。
あと間違ったパラメータを渡すとアプリケーションエラーが発生するので直しましょう、、
ユーザが間違ったパラメーターに編集するとエラーページに遷移するとは正しいと思ってますが、
一応リクエストパラメーターのバリデーションを追加します。パラメーターが正しいときだけ結果返すようにします。
Search用で使用するメソッドではないと思います
検索機能を実装したときこのメソッドにwhere処理を追加して、検索ボタンを押すとこのメソッドを呼んで結果をとるつもりですが、何かご懸念点ございますか？
There was a problem hiding this comment.
Shinさんがアドバイスした実装方法も考えてみましたが、今後たとえ並び方法が多くなったらコントローラー側は
def index
@tasks = Task.expiring(params[:sort]).latest(params[:sort]).newest(params[:sort])... ...
end
みたいになちゃいます。latest.newestみたいなソースを読むと意味わかりづらいので、
同じ機能のscopeを全部繋ぐのは避けたいということを考えてます。mm
There was a problem hiding this comment.
こういう書き方にするとユーザが任意の操作でデータベースの並び順を「変えれます」でした。。すみません誤字です。
この辺をもっと説明すると
taskで並び順を変えたいところ以外のカラムを入力しても動きます例えば現在はcreated_atを並び順変えを入れているのにdescriptionなど他のカラムをパラメーターに渡して入力しても並び順を変えれます。
こちらは開発者が意図してない並び順変更ができてしまい、
今後indexがない状態のカラムで膨大なデータがある場合、
並び順変更だけでtimeoutエラーとかDBへの負荷も高まる可能性があります。
あと意図してないカラムを他の人に漏出してしまうのも良くないと思うので
意図したカラム以外の入力はできなくしたいですね
あとアプリケーションエラーの補足説明ですが、
urlで入力するカラム名を変え入力すると500系エラーが表示されます。
一般にはアプリケーションの動作が止まることはないと思います404に移動するか、
並び順が変わったりなど動作は担保されるはずです。
There was a problem hiding this comment.
Shinさんがアドバイスした実装方法も考えてみましたが、今後たとえ並び方法が多くなったらコントローラー側は
def index @tasks = Task.expiring(params[:sort]).latest(params[:sort]).newest(params[:sort])... ... endみたいになちゃいます。
latest.newestみたいなソースを読むと意味わかりづらいので、 同じ機能のscopeを全部繋ぐのは避けたいということを考えてます。mm
sortと並び順をパラメータに渡すことでlatest、newestなどはなくせるのでは？
There was a problem hiding this comment.
scopeに全部書くを避けたいイメージはどれほどの列の並び変えを考えてます？
最初のちょうさんが書いた状態からasc descの変更ができればいいと思いましたが、、
|
@sourcewc |
https://github.com/Fablic/training/blob/develop/steps_jp.md#%E3%82%B9%E3%83%86%E3%83%83%E3%83%9712-%E7%B5%82%E4%BA%86%E6%9C%9F%E9%99%90%E3%82%92%E8%BF%BD%E5%8A%A0%E3%81%97%E3%82%88%E3%81%86--%E3%82%B9%E3%82%AD%E3%83%83%E3%83%97%E5%8F%AF%E8%83%BD