Zad 5 homework #143
Zad 5 homework #143
Conversation
| @@ -120,7 +125,11 @@ GEM | |||
| actionpack (>= 4.0) | |||
| activesupport (>= 4.0) | |||
| sprockets (>= 3.0.0) | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
Tu jakieś konflikty zostały :)
| @@ -1,7 +1,22 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class BuildingsController < ApplicationController | |||
| def index; end | |||
| def index | |||
| render json: Building.all.order(:id), include: [:warriors] | |||
There was a problem hiding this comment.
order po id jest chyba tym domyslnym :)
|
|
||
| # bulding cant be defended without warriors | ||
| if building.warriors.any? | ||
| warriors_count = building.warriors.where(type: 'Warriors::Samurai').count |
There was a problem hiding this comment.
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 :)
NP.:
Building.warriors.select { |w| w.type == 'Warriors::Samurai' }.lengthThere was a problem hiding this comment.
Swoją drogą, tutaj zamiast robić where na bazie typu, możnaby dodać nową relację w budynku z customowym where opisanym tak jak to zrobileś w tej linijce kodu i byśmy ich po prostu wyciągali tak:
building.samurais
building.hussars| end | ||
| def self.call(building:) | ||
| # this should stop the method if getting null | ||
| return unless building |
There was a problem hiding this comment.
Taką logikę wywołania serwisu pod jakimś warunkiem, zazwyczaj realizuje sie nie w samym serwisie, a nieco wyżej, tam gdzie chcemy wywołać serwis powinniśmy wrzucić takie sprawdzenie :) Dzięki takiemu rozwiązaniu klasę w przyszłości wygidniej się rozwija/modyfikuje
| @@ -0,0 +1,8 @@ | |||
| class CreateReporotsSingleReports < ActiveRecord::Migration[5.2] | |||
| def change | |||
| create_table :reporots_single_reports do |t| | |||
| def change | ||
| add_column :buildings, :default_rice_need, :integer, default: 0 | ||
|
|
||
| Buildings::Stronghold.find_each { |building| update_attribute(:default_rice_need,10) } |
There was a problem hiding this comment.
Czemu w sumie nie default: 10 na kolumnie? ;)
|
|
||
| module Reports | ||
| class SiegeReport | ||
| def self.check_siege_ability(building:) |
There was a problem hiding this comment.
Models to trochę złe miejsce na taki "util", do tego typu działań genialnie nadaje się ServiceObject :)
| if prev_building_id | ||
| Reports::SiegeReport.call(building: Building.find(prev_building_id)) | ||
| Reports::SiegeReport.call(building: Building.find(building_id)) | ||
| update_column(:prev_building_id, nil) |
There was a problem hiding this comment.
Z tego co pamiętam update_column omija walidację więc nie jest to najlepszy sposób na edycje kolumny :D Najprosciej byłoby dodać relację typu belongs_to z opcją optional: true :) Optional pozwoli na ustawianie nila na tej kolumnie, nie trzeba nic zmieniać w bazie, bo bazy domyślnie pozwalają na nile, a jedynie rails sam waliduje to że nie mogą być nilem :)
| expect(building.siege_ability).to eq(500) | ||
| end | ||
|
|
||
| # Fail got 250 instead of 500 |
There was a problem hiding this comment.
To może wynikać z tego, że dla building trzeba przeładować atrybuty i asocjacje z bazy za pomocą metody '.reload'
No description provided.