macropusgiganteus / scrappy-web

Project for Technical Test
0 stars 0 forks source link

[Chore] Code architecture – Controllers are assuming too much responsibilities #10

Open malparty opened 3 weeks ago

malparty commented 3 weeks ago

Issue

All the core business logic is in the KeywordsController controllers. This has many consequences, for example, is it harder to write unit tests, the business logic cannot be reused for other endpoints, controller's responsibilities are harder to read and understand due to the presence of additional code.

Expected

The code in controllers should be limited to the responsibilities of a controller. In Ruby on Rails, it is accepted to do a little bit more than in some frameworks (e.g. many middlewares like authentication will be "configured" in the controller and often implemented in a concern or in the ApplicationControler). Core business logic (Google Queries, HTML parsing, or even complex ActiveRecord queries) must be implemented in separate objects.

macropusgiganteus commented 3 weeks ago

Thank you for your feedback.

I understand that the controllers get bloated with too many responsibilities and we can move them into separate objects.

If I were to solve the problem I would:

  1. Create service objects for CSV reading, google queries and HTML parsing
  2. Convert complex ActiveRecord queries into model scope Before
    
    # app/models/keyword.rb
    class Keyword < ApplicationRecord
    belongs_to :user
    has_one :keyword_result
    end

app/controllers/keywords_controller.rb

@keywords = Keyword.includes(:keyword_result) .where("user_id = ? AND keyword ILIKE ?", session[:user_id], "%#{params[:query]}%")


_After_
```ruby
# app/models/keyword.rb
class Keyword < ApplicationRecord
  belongs_to :user
  has_one :keyword_result

  scope :by_user, ->(user_id) { where(user_id: user_id) }
  scope :search_by_keyword, ->(query) { where("keyword ILIKE ?", "%#{query}%") }
end

# app/controllers/keywords_controller.rb
@keywords = Keyword.includes(:keyword_result)
                   .by_user(session[:user_id])
                   .search_by_keyword(params[:query])

References: