Filtrando usuarios por criterios en Ruby on Rails -- ruby campo con ruby-on-rails campo con active-record camp codereview Relacionados El problema

Filtering users by criteria in Ruby on Rails


3
vote

problema

Español

Soy un desarrollador bastante nuevo (rubyonrails). Deseo mejorar mis habilidades de codificación, así que usé el clima para hacer alguna revisión en mi código. Me dio una bandera para este método que su complejo. ¿Se caracteriza como complejo debido a tener varias "acciones / tareas" en un solo método?

¿Será mejor si extrao algunos segmentos de código a un método diferente?

¿Hay algo más que no estoy viendo?

    def search     filter_mapping = {"cost" => "rcost", "quality" => "rquality", "time" => "rtime", "experience" => "rexperience", "communication" => "rcommunication"}     @filters = params[:filter].split(",") rescue []     @sort = params[:sort]      @user_type = params[:s_user_type]     @skills = params[:s_skills]     @location = params[:location]     @max_rate = params[:max_rate]     @availability = params[:availability]      @users = User.scoped.joins(:user_skills)     @users = @users.where('user_type = ?', @user_type) if @user_type.present?     @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0     @users = @users.where('availability = ?', @availability) if @availability.present?     @users = @users.where('location_country = ?', @location) if @location.present?     @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?     @users = @users.page(params[:page]).per(PER_PAGE)      @filters.each do |f|       if filter_mapping.keys.include?(f)         @users = @users.order("#{filter_mapping[f]} desc")       end     end      @users = @users.order('id asc') if @filters.empty?     @advanced_link = @location.blank? && @max_rate.blank? && @availability.blank?     render :index   end   

actualización

Me di cuenta de que puedo extraer los ámbitos en un método como ese:

    def get_users_where_scopes     @users = User.scoped.joins(:user_skills)     @users = @users.where('user_type = ?', @user_type) if @user_type.present?     @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0     @users = @users.where('availability = ?', @availability) if @availability.present?     @users = @users.where('location_country = ?', @location) if @location.present?     @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?     @users = @users.page(params[:page]).per(PER_PAGE)    end   

y luego llámalo con @users = get_users_where_scopes() . Pero ahora la complejidad de este método me parece mal.

Original en ingles

I am a fairly new (RubyonRails) developer. I desire to improve my coding skills so I used climate to do some review on my code. It gave me a flag for this method that its complex. Is it characterised as complex because of having several "actions/tasks" in a single method?

Will it be better if I extract some code segments to a different method?

Is there something else I am not seeing?

  def search     filter_mapping = {"cost" => "rcost", "quality" => "rquality", "time" => "rtime", "experience" => "rexperience", "communication" => "rcommunication"}     @filters = params[:filter].split(",") rescue []     @sort = params[:sort]      @user_type = params[:s_user_type]     @skills = params[:s_skills]     @location = params[:location]     @max_rate = params[:max_rate]     @availability = params[:availability]      @users = User.scoped.joins(:user_skills)     @users = @users.where('user_type = ?', @user_type) if @user_type.present?     @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0     @users = @users.where('availability = ?', @availability) if @availability.present?     @users = @users.where('location_country = ?', @location) if @location.present?     @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?     @users = @users.page(params[:page]).per(PER_PAGE)      @filters.each do |f|       if filter_mapping.keys.include?(f)         @users = @users.order("#{filter_mapping[f]} desc")       end     end      @users = @users.order('id asc') if @filters.empty?     @advanced_link = @location.blank? && @max_rate.blank? && @availability.blank?     render :index   end 

Update

I figured out that I can extract the scopes into a method like that:

  def get_users_where_scopes     @users = User.scoped.joins(:user_skills)     @users = @users.where('user_type = ?', @user_type) if @user_type.present?     @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0     @users = @users.where('availability = ?', @availability) if @availability.present?     @users = @users.where('location_country = ?', @location) if @location.present?     @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?     @users = @users.page(params[:page]).per(PER_PAGE)    end 

and then call it with @users = get_users_where_scopes(). But now the complexity of this method seems wrong to me.

        

Lista de respuestas

1
 
vote
vote
La mejor respuesta
 

Diría que primero debía hacer un objeto de servicio para mantener el controlador magro y limpiar, y para darse un lugar para poner toda la lógica sin temor a contaminar el controlador. Además: ¡es reutilizable!

  # app/services/user_search.rb  class UserSearch   ORDER_MAPPING = {     "cost"          => "rcost",     "quality"       => "rquality",     "time"          => "rtime",     "experience"    => "rexperience",     "communication" => "rcommunication"   }.freeze    def initialize(params)     @params = params   end    def results     @results ||= begin       records = User.scoped.joins(:user_skills)       records = scope(records)       records = order(records)     end   end    private    def param(key)     @params[key] if @params[key].present?   end    def scope(scoped)     scoped = add_scope(scoped, 'user_type = ?', param(:user_type))     scoped = add_scope(scoped, 'user_skills.skill_id in (?)', skill_ids)     scoped = add_scope(scoped, 'availability = ?', param(:availability))     scoped = add_scope(scoped, 'location_country = ?', param(:location))     scoped = add_scope(scoped, 'rate < ?', max_rate)   end    def add_scope(scope, sql, *params)     scope.where(sql, *params) if params.all?(&:present?)     scope   end    def order(scope)     terms = sanitized_order_terms || default_order_terms     terms.each { |term| scope.order(term) }     scope   end    def sanitized_order_terms     terms = param(:filter).try(:split, ",")     terms = terms.map { |term| ORDER_MAPPING[term] }     terms = terms.compact     terms if terms.any?   end    def default_order_terms     ["id asc"]   end    def skill_ids     param(:s_skills).try(:map, &:to_i)   end    def max_rate     param(:max_rate).try(:to_i)   end end   

He mantenido intencionalmente la paginación en el controlador, ya que es bastante independiente del alcance y el pedido. Sin embargo, sería fácil agregar como argumentos al método #results

en su controlador:

  def search   @users = UserSearch.new(params).results.page(params[:page]).per(PER_PAGE)    advanced_params = %w(location max_rate availability).map { |p| params[p] }   @advanced_link = advanced_params.all?(&:blank)    render :index end   

Probablemente elegiría una forma más directa de determinar el @advanced_link , como enviar un parámetro 9988777665544336 a lo largo, y simplemente mirando eso en lugar del estado implícito que tener ahora.

No tengo idea de qué código clima piensa del código anterior, pero me imagino que será más feliz.

 

I'd say to first make a service object to keep the controller lean and clean, and to give yourself a place to put all the logic without fear of polluting the controller. Plus: It's reusable!

# app/services/user_search.rb  class UserSearch   ORDER_MAPPING = {     "cost"          => "rcost",     "quality"       => "rquality",     "time"          => "rtime",     "experience"    => "rexperience",     "communication" => "rcommunication"   }.freeze    def initialize(params)     @params = params   end    def results     @results ||= begin       records = User.scoped.joins(:user_skills)       records = scope(records)       records = order(records)     end   end    private    def param(key)     @params[key] if @params[key].present?   end    def scope(scoped)     scoped = add_scope(scoped, 'user_type = ?', param(:user_type))     scoped = add_scope(scoped, 'user_skills.skill_id in (?)', skill_ids)     scoped = add_scope(scoped, 'availability = ?', param(:availability))     scoped = add_scope(scoped, 'location_country = ?', param(:location))     scoped = add_scope(scoped, 'rate < ?', max_rate)   end    def add_scope(scope, sql, *params)     scope.where(sql, *params) if params.all?(&:present?)     scope   end    def order(scope)     terms = sanitized_order_terms || default_order_terms     terms.each { |term| scope.order(term) }     scope   end    def sanitized_order_terms     terms = param(:filter).try(:split, ",")     terms = terms.map { |term| ORDER_MAPPING[term] }     terms = terms.compact     terms if terms.any?   end    def default_order_terms     ["id asc"]   end    def skill_ids     param(:s_skills).try(:map, &:to_i)   end    def max_rate     param(:max_rate).try(:to_i)   end end 

I've intentionally kept the pagination in the controller, as it's pretty independent of the scoping and ordering. However, it'd be simple to add as arguments to the #results method

In your controller:

def search   @users = UserSearch.new(params).results.page(params[:page]).per(PER_PAGE)    advanced_params = %w(location max_rate availability).map { |p| params[p] }   @advanced_link = advanced_params.all?(&:blank)    render :index end 

I'd probably pick a more direct way of determining the @advanced_link, such as sending an advanced parameter along, and simply looking at that instead of the implicit state you have now.

I have no idea what Code Climate thinks of the code above, but I imagine it'll be happier.

 
 
   
   

Relacionados problema

3  Semilla una base de datos  ( Seed a database ) 
Estoy buscando consejos / mejores prácticas para evitar hacer un conjunto profundo anidado de IF / 99887766555443333 en Rails. La intención es sembrar una...

4  Selección ActiverCord, modificada por parámetros opcionales  ( Activerecord selection modified by optional parameters ) 
Tengo un método de acción en My Rails Controller que filtra un modelo de ActiveRecord dependiendo de los parámetros de obtención enviados. rubocop se queja...

3  Los parámetros de limpieza antes de pasarlos a un modelo  ( Cleaning parameters before passing them to a model ) 
Estoy escribiendo una API y quiero poder pasar en los atributos de un modelo sin prefijarse con el nombre del modelo. Escribí esta pequeña extensión a (ns ...

5  Buscar usuario por primera y / o apellido, de manera eficiente  ( Search for user by first and or last name efficiently ) 
Estoy refactorizando una búsqueda de usuarios que solo se siente sucia. (Usuarios # Búsqueda) Necesito permitir que un param en blanco busque en parcial o s...

5  Una consulta de registro activo condicional  ( A conditional active record query ) 
Tengo una consulta de registro activo condicional que verifica si existe una relación de tipo x ( 99887776655544332 ) donde attribute x3 y attribute y4 En...

3  Consultas de actualización de MySQL Duplicate_key  ( Custom mysql duplicate key update queries ) 
El objetivo es crear una actividad o actualizar su contador si ya está presente. Y para ser súper precisa, el código evita las condiciones de carrera con un ...

0  Contando número de mensajes para un buzón durante varios períodos  ( Counting number of messages for a mailbox over several periods ) 
La idea es devolver una serie de mensajes recibidos hoy en día, esta semana, este mes. def index @mailboxes = current_user.mailboxes @today, @mon...

0  Diseño de Ruby OO: ¿Cómo manejar el estado inconsistente en una clase mutable?  ( Ruby oo design how to handle inconsistent state in a mutable class ) 
Resumen Me gustaría tener consejos sobre cómo: implementar métodos que se basan en un estado , cuando la instancia es mutable y puede estar en un est...

1  Agregue modelos existentes a una relación usando atributos anidados  ( Add existing models to a relation using nested attributes ) 
Necesitaba agregar modelos existentes a una relación con muchas personas mediante atributos anidados, por lo que sobrescribí thumbnails_attributes= : clas...

1  Copiando CSV a SQL_Table  ( Copying csv to sql table ) 
Escribí el código que lleva CSV y expórtelo a la tabla SQL. Parece esto: output_leftBound()1 Este método creará una tabla basada en la variable de nombr...




© 2022 respuesta.top Reservados todos los derechos. Centro de preguntas y respuestas reservados todos los derechos