Skip to content

Solve Zad5 - #136

Open
mjkrajsman wants to merge 3 commits into
DaftAcademy:masterfrom
mjkrajsman:zad5
Open

Solve Zad5#136
mjkrajsman wants to merge 3 commits into
DaftAcademy:masterfrom
mjkrajsman:zad5

Conversation

@mjkrajsman

Copy link
Copy Markdown

Większość testów przechodzi. W kodzie znajduje się komentarz z treścią komunikatów zwracanych przez te, które nie przeszły - na wypadek, gdybym nie zdążył poprawić. Póki co brakuje mi już pomysłów:

  1. ActionController::UrlGenerationError: No route matches {:action=>"/buildings", :controller=>"buildings",
    :name=>"ABCD", :type=>"Buildings::Walls"}
    w buildings_controller_spec.rb
  2. ActiveRecord::RecordInvalid: Validation failed: Name has already been taken w siege_report_spec.rb (próbowałem podstawiać na siłę inne imiona - zawsze drugi w danym teście dodany wojownik rzuca tym błędem)

class WarriorsController < ApplicationController
def show
render json: warrior, include: %i[weapon building]
render json: warrior, include: %i[weapon building], status: 200

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

200 nie jest domyślnym statusem? :D


def destroy
warrior.destroy!
head 204

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wydaje mi się że przez brak render tu i tak leciało 204 :)

scope :alive, -> { where('death_date IS NULL') }
scope :dead, -> { where('death_date IS NOT NULL') }

after_save :update_siege_abilities

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeśli usuniemy warriora z budynku, to w budynku siege ability nie zaktualizuje sie :(

daily_supplies = 10
if @building.warriors.alive.any?
@building.warriors.alive.each do |warrior|
daily_supplies += if warrior.type == 'Warriors::Hussar'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ładniej by to chyba wyglądało, jeśli Warrior miałby zaimplementowaną metodę/atrybut np. supply_burn_rate, który zwraca 1 jeśli warrior nie ma konia i 2 jeśli go ma, wtedy nie trzeba by myśleć nad takimi ifami w tym serwisie :)

before { get '/buildings' }
context 'with valid params' do
context 'with correct name' do
include_examples 'OK'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trochę mało czytelne nazwy tych shared examples, ale duży plus za zastosowanie takich technik! :D

def call
raise NotImprementedYet
daily_supplies = 10
if @building.warriors.alive.any?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sprawdzenie spokojnie by się mogło znaleźć gdzieś wyżej, żeby nie obciążać logiki serwisu :) Serwis najlepiej jeśli ma jedną odpowiedzialność, a Twój serwis poza liczeniem ile dni przetrwa dany budynek, stwierdza upadłość budynku jeśli nie ma w nim wojowników :)

factory :warrior, class: 'Warrior' do
association(:building)
association(:clan)
name { 'WarriorName' }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do problemu z imionami można zastosować konstruckję z sequence żeby zapewnić im unikalność :) NP.:

sequence :email do |n|
  "person#{n}@example.com"
end

end
end

# BuildingsController POST /buildings with valid params with correct name

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu chodzi o typ testu :) Dla testów typu controller jest trochę inna, powinno się w nich pisać np.:

get :index
post :create

Testy Ci zadziałają, jeśli zmienisz typ testu na request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants