A Big Project – Skinny Code Review


Evolution

Un post con título análogo a Fat Model – Skinny controller, lo he querido titular A Big Project – Skinny Code Review, ya que es mucho más productivo y gratificante, el exponer una pequeña parte del código, que no un montón de código que seguro sería eterno y se sacaría poco jugo. Bajo mi punto de vista, es muy importante que toda empresa, tenga implantada como materia obligatoria, “La revisión de código – Code Review”. Puede sonar un poco dura o asustar a cualquiera el hecho de salir y exponer o airear el trabajo que estás haciendo para que otros lo vean y opinen, pero en realidad es un fuente de conocimiento y positividad brutal. No se trata en mi opinión y esto debería ser en general, de ridiculizar al que expone, todo lo contrario, se trata de que todos opinemos y genere un debate sobre la mejora, si buscamos la excelencia. Pero créeme …. es una de las mejores experiencias por las que un desarrollador debe y tiene que pasar. Pero yo desde la sinceridad, seré y soy, un voluntario incondicional para salir a exponer mi trabajo, siempre que tenga la oportunidad.

Después de esta introducción, quería exponer un caso real, que en la última revisión de código que hemos tenido en ASPGems, he tenido que salir a exponer un trozo de código del proyecto en que actualmente estoy embarcado. Primero voy a contar el origen del porque he elegido un bloque de código determinado, del que luego hablaremos. Todo parte cuando, hablando una mañana con Gabriel Ortuño, le pregunté sobre el paso de parámetros en Ruby y le mostré los métodos que estaba preparando para resolver el reenvío de email a petición del usuario que está identificado dentro de la aplicación que nos pidió nuestro cliente. Cuando Gaby lo vio, me comentó: “me parece un poco extraño eso que estás haciendo, cuando llamas a ese método” explicándome los motivos de su sospecha y pensé “mi código huele”, sin entrar en mayor detalle, eso me llamó la atención y se me quedó ahí, en la recámara para eso que yo llamo: “tengo que hacer algo con esto”.

Bueno, el caso es que llegó el día de revisión de código y Javier Lafora, me pregunta ¿tienes algo que quieras mostrar en la revisión de código de hoy?,  tardé en responder 0 segundos🙂. Ya imaginas en qué código pensé y me vino a la mente rápidamente, verdad?. El trozo de código que expuse es parte de un modelo en el que como he dicho antes, debía resolver la posibilidad de que cuando un usuario está identificado dentro de la aplicación puede enviar o reenviar un email del estado de situación de un inmueble.

El código pertenece a un modelo y corresponde a los dos métodos expuestos a continuación y son “private”:

def send_informative_mail(to = user, cc = other_mail_users)
            HouseApprovalMailer.informative_mail(to, cc, self).deliver
end

def send_rejection_mail(to = user, cc = other_mail_users)
           HouseApprovalMailer.rejection_mail(to, cc, self).deliver
end

Estos dos métodos privados se encargan de enviar un mail informativo o un mail de informativo de rechazo de un determinado obtejo que se le porporciona. Estos dos métodos privados de send_informative_mail y send_rejection_mail,  son accesibles desde un método publico que a su vez es llamado dicho método desde el controlador. El método es:

def reply_reminder(house)

       if house.approved.nil?
             # deprecated_approvals = House::Approval.where(“approved IS NULL and house_id = ?”,house.id)
             # deprecated_approvals.each{|approval| approval.send_reminder(current_user.login)}
       elsif house.approved
            send_informative_mail(current_user.login, nil)
       else
            send_rejection_mail(current_user.login, nil)
       end

end

Lo primero que se plantea en la revisión, son las llamadas de send_informative_mail o send_rejection_mail con los dos parámetros del current_user y nil. La confusión parte realmente de ver nil, te preguntas ¿qué hace realmente? no queda claro. Debería hacerse la llamada al método por ejemplo: send_informative_mail(current_user.login) si únicamente queremos que se envíe un email informativo al usuario que ha accedido a la aplicación y si queremos enviar con copia a otros usuarios, debería ser send_informative_mail(current_user.login, other_user_mail), esto al leerlo quedaría mucho más claro, pero además nos quedaría la definición del método def send_informative_mail(to = user, cc = nil). Por tanto, ya vemos el cambio:

         elsif house.approved
            send_informative_mail(current_user.login)
         else
            send_rejection_mail(current_user.login)
        end

Después, si nos fijamos, está comentada la condición nil por no ser funcional, en ese caso, más aún para mejorarlo. Pero seguimos.

El siguiente punto a comentar sería las líneas de código que corresponden al if. Primero, el tema de  los estados, pueden ser “Pendiente”, “Aprobado” o “Rechazado” con los valores de nil, true o false. Podríamos mejorar los valores de nil, true o false creando una hash de pares clave valor. Utilizando el método state que nos sirve para decirnos el literal de: “Pendiente”, “Aprobado” o “Rechazado”:

                           STATES = {
                                   approved: 1,
                                   pending: 0,
                                   rejected: 2
                                  }

         def state
             return I18n.t(‘house.states.pending_state’) if approved.nil?

             return approved? ? I18n.t(‘house.states.approved_state’) I18n.t(‘house.states.rejected_state’)
         end

Ahora para mejorar el if, lo único que tendríamos que hacer sería quitarnos las líneas del if y cambiarlas por:

approved? ? send_informative_mail(current_user.login) : send_rejection_mail(current_user.login)

Con lo que finalmente se quedaría en una línea de código nuestro método de reply_alert, mejor y más claro, verad?. Otro apartado que hablamos a considerar es, cuando hablamos de estados, debemos pensar si nos interesa utilizar máquinas de estado o no, la pregunta es ¿los cambios de estado con con ejecuciones, actúan y nada más? o ¿pueden pasar por distintos estados de unos a otros?. Si respondemos a la primera afirmativamente como es este caso que estoy hablando, lo dejamos así, pero si la respuesta es afirmativa a la segunda, entonces piensa en utilizar algunas de las que hablo en este post Entender las máquinas de estado, State Machine, AASM y Workflow.

Finalmente el método que se encarga de enviar los mail, en este caso, de enviar un mail informativo de rechazo, es el siguiente:

def rejection_mail(to, cc, house)
     email = user_mail(to) ———–buscamos el email del usuario
     cc = Array[cc].compact.map {|u| user_mail(u) } – [email]
     @house = house
     mail(:to => email, :cc => cc, :subject => I18n.t(“approval_mailer.rejection_mail.#{@house.house_type}.subject”,

             house_name: house.name), :from => “aprobaciones@cliente.com”)
end

También se habló de lo siguiente. El parámetro house que llega al método def reply_alert(house), pertenece al mismo modelo, no sería necesario pasar house como parámetro, ya que el objeto está cargado. Por tanto desde el controlador cuando se hace la llamada al método ya tenemos el objeto cargado y no es necesario hacer el paso de parámetro anteriormente mencionado:

def reply_email_house
         response = form_response do
                      house = House::house.find(params[:houseId])
                      if house
                            house.reply_reminder(house) habría que cambiarlo por house.reply_reminder
                            { success: true }
                       else
                            { success: false, errors: { reason: I18n.t(‘components.house.columns.delete_house_error’)}}
                       end
         end
         render :json => response
end

Para terminar, hablamos sobre que estamos involucrando al modelo en tareas de envío de mail, sería mucho mejor el pasarlo al controlador y que el modelo no se haga cargo de tareas que no necesita saber.

Consejo: siempre hay que estar en constante mejora, utiliza todas las herramientas posibles de comunicación para preguntar y también para ayudar a los demás

Responder

Introduce tus datos o haz clic en un icono para iniciar sesión:

Logo de WordPress.com

Estás comentando usando tu cuenta de WordPress.com. Cerrar sesión / Cambiar )

Imagen de Twitter

Estás comentando usando tu cuenta de Twitter. Cerrar sesión / Cambiar )

Foto de Facebook

Estás comentando usando tu cuenta de Facebook. Cerrar sesión / Cambiar )

Google+ photo

Estás comentando usando tu cuenta de Google+. Cerrar sesión / Cambiar )

Conectando a %s