Skip to content

Zad5#145

Open
gitpetrova wants to merge 18 commits intoDaftAcademy:masterfrom
gitpetrova:zad5
Open

Zad5#145
gitpetrova wants to merge 18 commits intoDaftAcademy:masterfrom
gitpetrova:zad5

Conversation

@gitpetrova
Copy link
Copy Markdown

                /\

/vvvvvvvvvvvv --------------------------------------------------------------
`^^^^^^^^^^^^ /=====================================/
/
katana zamiast opisu. Testy w models nie są częścią zadania. Używałem ich do TDD i zostawiłem.

validates :granary, presence: true,
numericality: { greater_than_or_equal_to: 0, only_integer: true }
validates :name, presence: true
validates :granary, :siege_ability, :horse_units, :infantry_units, presence: true,
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.

siege_ability zostało u Ciebie zaimplementowane jako obliczalny atrybut, więc nie ma co go w bazie przechowywać :) Oczywiście optyamlizacyjnie to nie brzmi najlepiej, jeśli chcielibyśmy to optymalizować to możnaby np. raz dziennie przy pomocy jakiejść kolejki(np. Sidekiq, ActiveJob, Resque) odświeżać ten atrybut, albo usunąć ten atrybut i w serializerze scachować wywołanie serwisu, żeby nie latał ciągle jak głupi :D

module Warriors
class Hussar < Warrior
attribute :preferred_weapon_kind, :string, default: :ranged
before_create :give_him_horse
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.

Obecne API chyba pozwala na zmianę tego atrybutu, przez co można zabrać husarzowi konia :( możnaby to rozwiazać walidacją, gdzie horse zawsze musi być true i dodatkowo zamienić ten callback na before_commit, co nie jest ładne dla klienta bo niby wszystko jest ok, a jednak, zmiana którą podał się nie wprowadza :<

has_many :warriors

def type
object.type.split('::')[1]
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.

Metoda demodulize jest trochę mądrzejsza :D

end

def warriors_alive
object.warriors.alive.count
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.

Tutaj mamy problem z n+1. Można by go rozwiązać przez bazodanowe wcześniejsze doincludowanie(#includes) wojowników do query po budynki, użycie select zamiast where do odfiltrowania żywych wojowników i użycie metody .length do liczenia :)

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.

NP.:

object.warriors.select { |w| !w.death_date }.length

end

def call
@params_key.to_i == 0 ? @warriors.dead : @warriors.alive
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 mogłeś skorzystać ze zdefiniowanych readerów

RSpec.describe Building, type: :model do
describe 'creating new buildings' do
before(:each) do
@clan = create(:clan, id: 1, name: 'Klana')
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.

Zdecydowanie ładniej by to wyglądało przy wykorzystaniu let! lub let z wywołaniem w before :)

describe 'creating new buildings' do
before(:each) do
@clan = create(:clan, id: 1, name: 'Klana')
@building = create(:building, granary: 100)
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.

Jako że test dotyczy budynku, do tej zmiennej zamiast wspomnianego wyżej let, można by użyć helpera o nazwie subject do definicji :)

RSpec.describe Reports::SiegeReport do
subject(:siege_report) { Reports::SiegeReport.new(building: building).call }

let(:building) { create(:building, granary: 100) }
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.

Można by jeszcze sprawdzić sytuację, gdzie spichlerz nie ma żadnych zasobów :)

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