Coś tam trochę kąsam z tych railsów, ale określiłbym to jako zaledwie podstawy.
Natomiast chciałbym podwyższyć skilla a najlepszą metodą jest wystawić się na ogień krytyki (słynna metoda określania jakości kodu - ilość WTF? na minutę ).
Info: Nie jest to kolejny blog ani twitter, są to zakłady (bukmacherskie) tyle że obstawiane w bitcoinach. Prawie kompletne rozwiązanie - brakuje kilku funkcjonalności, szczególnie w adminie. Technicznie jest dosyć proste, bo nie ma Devise, Hamla, zaawansowanych wzorców jak dekoratory czy prezentery itp.
Interesują mnie wszelkie opinie o tym kodzie, od takich, że słaby (wiem), po jakieś konkretne rady, co tam dodać/ulepszyć/zmienić, będę wdzięczny jak kilka osób rzuci na to okiem i jakoś to oceni, albo doradzi nad czym popracować.
Z góry dzięki!
OperationsController chyba powinien sie nazywac WithdrawalsController ? bo sluzy (przynajmniej na razie) do robienia withdrawali
logike z akcji create przerzucilbym poza kontroler (czyli do jakies osobnej klasy, specjalnie unikam sformulowania ‘do modelu’ bo zalezy kto co rozumie przez ‘model’)
podobnie w UsersController#create logike zwiazana z tworzeniem konta bitcoin tez bym wyrzucil - to jest integralna czesc operacji ‘utworz nowego uzytkownika’ i nie zalezy od tego, z ktorego kontrolera ten ‘flow’ jest wolany. Mogloby siedziec w kawalku nadpisanej metody save dla Usera wolanej w przypadku tworzenia nowego rekordu (w duchu odchodzenia od AR callbackow) albo w jeszcze osobnej klasie
@slawosz: jeśli chodzi Ci o to, żeby zrobić działającą apkę, to problem jest taki, że musi być uruchomiony klient bitcoin (bitcoind). Mam serwer, gdzie można to odpalić, ale musiałbym się bawić z Passengerem, albo coś instalować, bo to LAMP Ale pomyślę nad tym.
O state machine właśnie myślę - tutaj mi ktoś nawet zasugerował w innym wątku.
A formatowanie muszę poprawić faktycznie. Miałem źle ustawione tabulatory na początku
Twoja aplikacja tak naprawdę zawiera dość poważny błąd. Otóż kiedy zlecisz bardzo szybko kilka wypłat na raz możliwe jest, że aplikacja nie zdąży jeszcze odjąć salda i będzie możliwe zrealizowanie kilku wypłat na raz mimo salda pozwalającego na tylko jedną wypłatę. Poczytaj o transakcjach i blokowaniu rekordów w bazie danych.
wg MVC i Model2 (używanego w RoR) to w modelu przechowuje się logikę biznesową, a to że community railsów utożsamia ActiveRecord z modelem, a MongoMapper/MonogoId z dokumentem, to jest dla mnie niezrozumiałem. Wszystko jest modelami (również services znane z artykułu CodeClimate), trzeba jednak zrozumieć, że modelach jest kilka warstw i problem znika.
Operacji jest w sumie 5, ale faktycznie z tego kontrolera jest tylko zlecanie wypłaty. Ale zgadzam się, że można kilka rzeczy przeprojektować, szczególnie w kontrolerach.
@nvll, słuszna uwaga. W komercyjnym produkcie to byłby spory błąd. Zresztą tak samo jest przy rozliczaniu zakładu przez admina - tam co prawda ciężko jest zlecić kilka operacji szybko po sobie, ale też każdy “przelew” składa się z 2 elementów (zmiana salda w kliencie i zapis tej operacji w bazie).
Będę to oczywiście poprawiał, ale nie będę informował o każdej poprawce, żeby nie spamować. Wrzucę info jak będzie trochę więcej zmian.
mozesz napisac fake’owego clienta bitcoina do testow
mysle ze spokojnie mozesz spamowac forum zmianami, nikt sie nie obrazi, a po odzewie ludzi widac ze tylko beda pomagac - najlepiej rob pull requesty dla kazdej zmiany, by bylo latwo robic review
Integralna, ale nie wymagana (tzn. można utworzyć konto usera nawet gdy klient btc nie działa, będzie miało ograniczoną funkcjonalność dopóki się tego nie uzupełni). Pierwotnie miałem to w callbacku, ale 1) testowanie modelu wymagało mockowania klienta lub przynajmniej metody, 2) ciężej było wyłapywać wyjątki i wyrzucać info dla end usera co się dzieje.
Podoba mi się idea osobnej klasy, tylko nie wiem jak to fachowo ugryźć
Coś na zasadzie dekoratora?
def create
@user = UserAccount(User.new(user_params))
if @user.save
sign_in @user
flash...
redirect...
else
render....
end
end
Wówczas UserAccount miałby metodę save, która by najpierw wywołała User.save a potem próbowała utworzyć konto bitcoin. Kontroler prosty, tylko jak wyrzucić notice lub error. No i User.save mogłoby dać true a UserAccount.save już niekoniecznie a wtedy kłopot z redirectem.
A może to w ogóle nie tędy droga?
Próbowałem kiedyś przerzucić logikę z kontrolera do modelu i skończyło się na tym, że z jednej strony kod w całości można było testować jednostkowo ale z drugiej strony, po jakimś czasie powrotu do aplikacji, to było bardzo nie czytelne. Tak jak jest teraz (ten fragment kodu co zamieściłeś) to z może być.
Przy tym podejsciu UserAccount robi sie ‘fasadą’ (a nie dekoratorem) dla całego ‘podprocesu’ i raczej powinien również zwrócić true / false - to co w srodku, staje sie z zewnetrznego pktu widzenia operacją atomową. A w zaleznosci od wyniku kontroler robi to co chce (redirect, notice, whatever).
Co do nazewnictwa: mozna spojrzec na to jak na ‘dodanie Usera do kolekcji / grupy Userow’
@user = User.new params[:user]
if Users.add(@user)
...
else
...
end
albo jako na rejestracje usera
@user = User.new params[:user]
if UserRegistration.new(@user).process
...
else
....
end
@konole, @mark: tak, ta metoda jest zdecydowanie do poprawy. Ona jest generalnie prawie żywcem przepisana z php - sam algorytm działa(ł) w miarę poprawnie, chociaż zapewne znajdzie się jeszcze kilka edge case’ów, dla których warto go przetestować.
Ale żeby to rozbić potrzebuję mieć testy. A żeby mieć testy, to dobrze by było mieć fake clienta do podstawienia. I właśnie to zacząłem robić.
To jest pierwsze podejście, żeby sprawdzić sam tok rozumowania.
Założenia:
nie ruszałem samego kodu aplikacji, z wyjątkiem dodania wyłapywania wyjątku do AdminController#transactions,
utworzyłem nowy moduł (Bitcoin) a w nim szkielet klasy FakeClient. Docelowo do tego modułu przeniosę obecny BitcoinClient (pewnie po prostu jako Bitcoin::Client),
FakeClient można zainicjalizować jako “nie działający”, żeby testować wyjątki.
Dobrze by było, żeby odpowiedniego klienta ustawiać w zależności od środowiska (coś jak tu: http://blog.snap-ci.com/blog/2013/11/18/local-rails-development-with-service-stubs/), ale nie wiem, jak wtedy testować sytuacje, gdzie klient nie pracuje (może domyślnie testować działający a osobny test przeznaczyć na testowanie wyjątków i tam go ustawiać ręcznie)?
Kolejna sprawa - metoda AdminController#transactions z założenia ma być wykonywana cyklicznie (cron, whenever) - jakieś sugestie, jak to rozwiązać (żeby też pozostawić możliwość wykonywania tego ręcznie)?
).
