Załóżmy, że mamy do zaimplementowania delegowanie jakiejś sprawy między dwoma współpracownikami. Reguła biznesowa jest taka, że użytkownicy muszą pełnić tą samą rolę by oddelegować do kogoś zadanie.
To jest nasz punkt wyjścia. Dyskusyjne rozwiązanie ale przyjmijmy, że działa.
class Delegation < ActiveRecord::Base
belongs_to :problem
belongs_to :new_user, class_name: :User
belongs_to :old_user, class_name: :User
validate :new_user_role_and_identity
private
def new_user_role_and_identity
errors.add(:new_user, :invalid) unless possible_new_users.include?(new_user)
end
def possible_new_users
User.with_role_of(old_user).where(
User.arel_table[:id].not_eq(old_user.id)
)
rescue
[]
end
endSytuacja wygląda teraz tak, że za każdym razem kiedy chce sobie w testach porobić coś z moim Delegation muszę użyć #create zamiast #build ponieważ moja walidacja korzysta z bazy danych. W ten sposób testy z czasem robią się wolne a my jako programiści jesteśmy mniej wydajni.
Mogę zestubować prywatną metodę new_user_role_and_identity albo possible_new_users. Bullshit! Nie powinno się stubować prywatnych metod. Mamy je w dupie. Dlaczego miałbym to robić ?
Ok, to zestubujmy wywołania metod na User. Jeszcze większy bullshit. Co jak się zmienią. Dlaczego każdy kto chce skorzystać z Delegation w testach miałby się tym przejmować i wiedzieć co ona pod spodem robi. Brzmi równie źle i głupio.
Ok, więc myślę sobie, o tym co opowiadałem na DRUGu po wroc_love.rb i walidatory powinny być zewnętrzne względem obiektu. Jak w Aequitas.
Więc napiszmy sobie jakiś prosty pseudo-walidator.
class DelegationValidator
def valid?(delegation)
possible_new_users(delegation).include? delegation.new_user
end
private
def possible_new_users(delegation)
User.with_role_of(delegation.old_user).where(
User.arel_table[:id].not_eq(delegation.old_user.id)
)
rescue
[]
end
endCo się zmieniło ? Aktualnie nic, problem dalej jest ten sam tylko przeniesiony gdzie indziej.
Może sytuacja byłaby prostsza gdyby ten walidator dostawał te dane, które są potrzebne do sprawdzenia?
class DelegationValidator
def initialize(possible_new_users)
@possible_new_users = possible_new_users
end
def valid?(delegation)
@possible_new_users.include? delegation.new_user
end
endNie takie złe. Teraz mamy inny problem. Każde miejsce, które chciałoby sprawdzić obiekt Delegation musiałoby tworząc taki DelegationValidator podawać mu listę dobrych osób do oddelegowania zadania. Doh. Jeśli takie miejsce jest jedno to nie niekoniecznie problem (wciąż, czy to dobrze miejsce jedną warstwę wyżej np Controller by takie obliczenia przeprowadzać i dostarczać? Chyba niekoniecznie). Jeśli takich miejsc jest więcej to mielibyśmy powtórzoną logikę tego. Wciąż fail.
Czyli może zatem DelegationValidator powinien zamiast korzystać z gotowego zestawu użytkowników, to kooperować z jakimś obiektem, który by mu tą wiedzę dostarczył w zależności od potrzeb ?
Boom:
class DelegationValidator
attr_reader :delegation_users_provider
delegate :possible_new_users_for, to: :delegation_users_provider
def initialize(delegation_users_provider = DelegationAllowedUsers.new)
@delegation_users_provider = delegation_users_provider
end
def valid?(delegation)
possible_new_users_for(delegation).include? delegation.new_user
end
end
class DelegationAllowedUsers
def possible_new_users_for(delegation)
User.with_role_of(delegation.old_user).where(
User.arel_table[:id].not_eq(delegation.old_user.id)
)
rescue
[]
end
endCzyżbym odkrył i zlokalizował dependency w toku tych rozważań ? Wygląda i brzmi nieźle. DelegationAllowedUsers wydaje się spełniać SRP. DelegationValidator ma jasno zdeklarowaną zależność, którą mogę w testach podstawiać dowolnie i być może nie będę musiał dzięki temu dotykać bazy bo mogę podstawić jakiś stub/mock, który powie true/false albo przeprowadzi to samo obliczenie w pamięci.
Jeśli więc miałem na początku trzy testy dla Delegation. Jeden sprawdzający, że dostanę informację o błędzie w sytuacji X a drugi w sytuacji, gdy oddelegowano do użytkownika o tej odmiennej roli w systemie a trzeci o braku błędu gdy o takiej samej. To za każdym razem musiałem tykać bazy.
Teraz mogę w każdym z testów podstawić odpowiednią spreparowaną zależność do DelegationValidator. Natomiast sam DelegationAllowedUsers przetestować w sytuacji pozytywnej i negatywnej osobno w innych testach, które niech im tam będzie, używają bazy. Im więcej testów walidacji nie związanych z samym przypadkiem badania, czy osoby mają taką samą rolę czy nie, tym większy win wydajnościowy.
- Czy naprawdę wygrałem ?
- Czy to jest lepszy kod niż hakerskie rozwiązania zaproponowane na początku ?
- Wg Ciebie ?
- Wg Fowlera ?
- Wg DHH ?
Czy walidacje zaimplementowane tak jak w AR mają jakikolwiek sens jeśli nie ma prostych sposobów na ekstrakcję takich dependency ? No chyba, że mówimy o:
class Delegation < ActiveRecord::Base
attr_accessor :delegation_users_provider
validate :new_user_role_and_identity
def new_user_role_and_identity
errors.add(:new_user, :invalid) unless possible_new_user?(new_user)
end
def possible_new_user?(new_user)
delegation_users_provider.possible_new_users.include?(new_user)
end
endi ustawianiu tej wartości na obiekcie klasy Delegation samym w sobie: delegation.delegation_users_provider = DelegationAllowedUsers.new; delegation.valid?. Co śmierdzi mi na kilometr również ...
Co sądzicie ?
- Wywód zakłada, że nie możemy sprawdzić
new_user.role == old_user.role;) - Czemu nie hackpad? Bo nie ma tam ładnego kodu.
