Kod działa ale zastanawiam się czy można go jakoś poprawić, uprościć czy zakres ads_by_user jest poprawnie napisany i jak do niego dodać informację o błędnym zakresie.
[code=ruby]#model
scope :confirmed_ads, :conditions => [“advertiser_id IS NOT NULL”]
scope :unconfirmed_ads, :conditions => [“advertiser_id IS NULL”]
scope :unverified_ads, :conditions => [“advertiser_id IS NOT NULL AND verification_date IS NOT NULL”]
scope :ads_by_user, lambda { |email| { :conditions => [“email = ?”, email ] } }
def self.with_some_scope(scope_name, email)
case scope_name
when ‘unverified_ads’
self.unverified_ads
when ‘unconfirmed_ads’
self.unconfirmed_ads
when ‘confirmed_ads’
self.confirmed_ads
when ‘ads_by_user’
self.ads_by_user(email)
else #domyslny scope
self.unverified_ads
end
end #kontroler @ads = Ad.with_some_scope(params[:scope], params[:email]) #widok
= link_to “Ogłoszenia niepotwierdzone”, verifications_path(:scope => “unconfirmed_ads”)
…
= link_to “#{ad.name}”, verifications_path(:scope => “ads_by_user”, :email => ad.email)[/code]
Nie wiem po co przeniosłeś wyrażenie case do modelu. Przecież tutaj nie ma żadnej logiki biznesowej. Jeśli dobrze rozumiem twoje wymagania to ten kod powinien zadziałać:
def index
@ads = case params[:scope]
when nil
#domyslny scope
Ads.unverified_ads
when 'unverified_ads'
Ads.unverified_ads
when 'unconfirmed_ads'
Ads.unconfirmed_ads
when 'confirmed_ads'
Ads.confirmed_ads
when 'ads_by_user'
Ads.ads._by_user(params[:email])
else
flash.now[:error] = "Błędny zakres"
nil
end
end
A powtórzeniami w kodzie się nie martw, przecież nie zawsze parametr zapytania musi się nazywać tak samo jak scope (nie ma potrzeby udostępniania metod modelu na zewnątrz).
def index
@ads = case params[:scope]
when nil
#domyslny scope
Ads.unverified_ads
when 'unconfirmed_ads'
Ads.unconfirmed_ads
when 'confirmed_ads'
Ads.confirmed_ads
when 'ads_by_user'
Ads.ads._by_user(params[:email])
else
flash.now[:error] = "Błędny zakres"
nil
end
end
Dlatego przeniosłem do modelu, bo nauczyłem się ‘fat model - thiny controller’ to się nie łapie?
[quote=knife]Nie wiem po co przeniosłeś wyrażenie case do modelu. Przecież tutaj nie ma żadnej logiki biznesowej. Jeśli dobrze rozumiem twoje wymagania to ten kod powinien zadziałać:
def index
@ads = case params[:scope]
when nil
#domyslny scope
Ads.unverified_ads
when 'unverified_ads'
Ads.unverified_ads
when 'unconfirmed_ads'
Ads.unconfirmed_ads
when 'confirmed_ads'
Ads.confirmed_ads
when 'ads_by_user'
Ads.ads._by_user(params[:email])
else
flash.now[:error] = "Błędny zakres"
nil
end
end
A powtórzeniami w kodzie się nie martw, przecież nie zawsze parametr zapytania musi się nazywać tak samo jak scope (nie ma potrzeby udostępniania metod modelu na zewnątrz).[/quote]
Taaa, a potem sobie napisz do tego testy.
@regedarek
Nie możesz tak skrócić.
A jak będzie zapytanie scope=unverified_ads to co wtedy?
Akcje w kontrolerach nie zawsze muszą być jednolinijkowe (powinny być po prostu zwięzłe).
@apohllo
Możesz napisać konkretnie dlaczego według Ciebie ten kod jest zły?
Po pierwsze - jeśli gdzieś indziej będziemy tego chcieli użyć (w innym kontrolerze, w innej akcji), to pozostaje kopiuj-wklej. No i tak jak pisałem - przetestowanie tego wymaga testów kontrolera (co angażuje szerszy kontekst).
Nie trafiają do mnie te argumenty.
Za pomocą dziedziczenia i metod prywatnych możemy uniknąć kopiowania kodu w kontrolerach.
Druga kwestia - nie powinno się przesuwać kodu z kontrolera do modelu tylko dlatego, że model łatwiej jest testować.
Sprawa jest jak widać dyskusyjna.
Moim zdaniem wybór danych w zależności od parametru zapytania należy do odpowiedzialności kontrolera.
[quote=knife]Nie trafiają do mnie te argumenty.
Za pomocą dziedziczenia i metod prywatnych możemy uniknąć kopiowania kodu w kontrolerach.
Druga kwestia - nie powinno się przesuwać kodu z kontrolera do modelu tylko dlatego, że model łatwiej jest testować.
Argumenty przeciw temu aby wrzucać to do kontrolera już zostały podane, mógłbyś przedstawić argumenty za ? Coś więcej oprócz “moim zdaniem” albo “to nie jest logika biznesowa”
[quote=regedarek]@hubertlepicki
Gdzie umieścić taką klasę?[/quote]
Gdziekolwiek, np. app/utils. Jeśli czujesz że to nie powinno być w samym modelu, chociaż moim zdaniem może tam być akurat to, to robisz sobie nową klasę która daną logikę obsługuje.
Twój problem może nie jest najlepszym do zobrazowania tego, ale nie zapominajmy że w Railsach możemy sobie tworzyć nowe obiekty nie będące modelami, widokami czy kontrolerami.
Dla mnie to jest ewidentinie model wyszukiwania / filtrowania. W najprostszym casie wystarczy meta_search / ransack, w bardziej skomplikowanym warto samemu napisać obiekt który będzie odpowiednio całą logikę wyszukiwania obsługiwał. Np z walidacjami nawet takiego formularza. http://drug-activemodel-presentation.heroku.com/ , http://rails3-active-model-filter.heroku.com/ .
