search

oleksiimikhno / RubyHW

0 stars 0 forks source link

Feedback HW 8 #11

Open LVALL opened 1 year ago

LVALL commented 1 year ago

1) Для пошуку в title або body Article створено допоміжний скоуп, чого можна було уникнути.

scope :find_in_column, ->(phrase, column) { where("#{column} ILIKE ?", "%#{phrase}%") }
scope :filter_by_phrase, ->(phrase) { find_in_column(phrase, :title).or(find_in_column(phrase, :body)) }

умову з or можна прописати з логічним || і працюватиме все так само, тільки без зайвого скоупу. 'title || body ILIKE ?'

також намагайтесь уникати інтерполяції при написанні sql запитів. Детальніше можна прочитати за посиланням

2) Фільтер в контролері реалізований коректно і охайно, але не варто всі можливі фільтри виносити в скоупи на рівень моделі. До прикладу scope :filter_by_status, ->(status_name) { where(status: status_name) } використовуватиметься лише в межах одного методу контролера, тому в методі index можна було зробити @articles = @articles.where(status: params[:status]) if params[:status].present? На даний момент не критично, але варто пам'ятати, що в моделі ми зберігаємо бізнес-логіку, скоупи, лише ті, які нам будуть потрібні в багатьох місцях і загалом пишемо в моделях якомога менше коду.

3) Фільтр tags має фільтрувати по кількох значеннях. - знову створено допоміжний скоуп, який більше ніде не буде використовуватись та якого можна було уникнути і який використовує рубішні методи, що у написанні запитів не є хорошою практикою.

  scope :serialize_tags, ->(tags) { where(tags: { name: tags.split(',').collect { |tag| tag.strip.downcase } }) }
  scope :filter_by_tags, ->(tags) { joins(:tags).serialize_tags(tags).distinct }

Можна зробити з ILIKE ANY,)

або scope :filter_by_tags, ->(tags) { joins(:tags).where(tags: { title: tags }) } Nested queries

4) ArticleSerializer - єдиний серіалайзер для Article, який використовується в усіх методах, у ньому через асоціації підтягується автор та коменти, що не є потрібним для всіх методів, окрім show. Зайві дані - зайвий час в обробці запиту та більше навантаження на сервер. Потрібно створювати окремі серіалайзери під потреби методу і називати відповідно, в даному випадку, хоча б зробити ArticlesSerializer - для індекс методу, де будуть додані асоціації та ArticleSerializer - для всіх методів, де повертається не колекція, а окремий об'єкт.

5) В ArticleSerializer тричі записана асоціація has_many :comments, але з різними скоупами. Серіалайзери створені для того, щоб просто отримати колекцію чи об'єкт та повернути вхідні дані в описаному форматі. Всередині серіалайзера фільтрація, пошук, чи будь-які дії з об'єктом відбуватись не повинні.

oleksiimikhno commented 1 year ago

1) Виправив на scope :filter_by_phrase, ->(phrase) { where('title || body ILIKE ?', '%' + phrase + '%') }

був ще варіант з concat, але він як для мене меньш охайний scope :filter_by_phrase, ->(phrase) { where("title || body ILIKE CONCAT('%',?,'%')", phrase) }

ще один варіант scope :filter_by_phrase, ->(phrase) { where('title || body ILIKE ?', "%#{sanitize_sql_like(phrase)}%") }

він єкранує запроси з % і _ link, але я невпевнений що він робить з скобками ' " ` хоча помилки при запросі не виникає як в цьому відео

можливо ви підскажете який найкращий варіант?

  1. Переніс всю логіку з статусом (комментів і артіклів) в контроллер

  2. Переробив на варіант scope :filter_by_tags, ->(tags) { joins(:tags).where(tags: { title: tags }) }

  3. Добре створив окремі серіалайзери, щоб виводили тільки паблік/анпаблік комменти для артикля /api/v1/articles/{id}/published і /api/v1/articles/{id}/unpublished

  4. Прибрав скопи, щоб не було фільтрацій. Просто було цікаво реалізувати, не знав що так не можна) Дякую за відгук