VladFomenko / RubyHW

0 stars 0 forks source link

Feedback for HW 8 #22

Open MaksHostroushko opened 1 year ago

MaksHostroushko commented 1 year ago

Thanks for you your work! Let's add some improvements

  1. https://github.com/VladFomenko/RubyHW/blob/main/HW8/app/controllers/api/v1/articles_controller.rb#L62..L69 I believe, could be better if we move this logic to a model like self.method In this case, we will be able to reuse this logic in different places without duplication Also, we should use articles instead of article, because Article.where return multiple records. So variable should be in plural Finally, we don't need to check articles.blank?. We just can render results render json: articles, status: :ok

  2. https://github.com/VladFomenko/RubyHW/blob/main/HW8/app/controllers/api/v1/articles_controller.rb#L71..L82 This code is a little bit unreadable Let's avoid duplications here to fix it Also, I think you can save results to articles variable and do render json: variables at the end of the file only once

  3. Please, move the logic for search and filtering to Articles#index

4.https://github.com/VladFomenko/RubyHW/blob/main/HW8/app/serializers/api/v1/article_serializer.rb#L7 You can just pass an object, not some parameters. I mean, you can pass author(not author_id) to serializer https://stackoverflow.com/questions/47565916/activemodel-serializer-passing-params-to-serializers

VladFomenko commented 1 year ago

@MaksHostroushko

  1. But if someone didn't find some articles, then returned status - status: :ok ?
  2. Do I need to move my all methods with sorting, searching and filtering to the index? Did I understand correctly?
  3. I don't use only object(without object.author_id), because object in article it is article. That's how I get this data.
#<Article:0x00007f2701a9c408
 id: 1,
 title: "Sharell Abbott",
 body: "Dangerous Fly",
 created_at: Wed, 14 Dec 2022 05:20:00.598885000 UTC +00:00,
 updated_at: Wed, 14 Dec 2022 05:20:00.598885000 UTC +00:00,
 author_id: 3,
 status: "published">

Author.find(object.author_id).name Author.find(author) - it doesn't work, because this object of the article