Crónicas de un Desarrollador

Continuando con esta serie de ejemplos de como refactorizar métodos de controladores vamos a seguir ahora con uno que cuando lo volví a ver apestaba feo feo.

El método en cuestión tiene por objetivo que el usuario actual se suscriba (es decir, anote) para jugar un partido determinado. Para eso el siguiente código es el que está siendo ejecutado actualmente :

  # app/controllers/matches_controllers.rb
  def subscribe
    flash[:notice] = "Ya formás parte del equipo!" and 
        redirect_to match_path(@match) and 
        return if @match.has_player? current_user

    flash[:error] = "El equipo está completo." 
      and redirect_to match_path(@match) and ]
      return if @match.available_places(params[:team].to_i)  current_user, :team => params[:team]

    redirect_to match_path(@match)
  end

Lo feo del problema se puede ver desde dos perspectivas : una que es una excusa y otra que es la razón de la desprolijidad. La excusa para este código tan horrible es que en caso de no poder agregar el jugador al equipo elegido tengo que mostrar un mensaje de error y redireccionar. Pero el redirect_to lo que hace es setear un header nada más, es decir que no hace un return auto-mágico, y de no hacerlo yo el segundo redirect_to (ubicado al final del código) haría que se lance una Exception por haber reenviado los headers.

El problema real es que estoy delegando en el controlador la responsabilidad de la clase Player para determinar si es válido crear una instancia para un partido y usuario dado.

Es por eso que lo primero que debemos hacer es usar los validators de Rails para esta tarea :

  # app/models/player.rb
  def validate
    errors.add_to_base("Ya formás parte del equipo!") if match.has_player? user
    errors.add_to_base("El equipo está completo.") if match.available_places(team) < = 0
  end

En este caso los errores no dependen de un atributo, por lo que usamos add_to_base para que los errores digan lo que queremos. Este método validate es llamado por Rails al crear o actualizar una instancia de Player, y si hay algún error nunca llega a la DB.

Habiendo quitado la validación del controlador ahora podemos escribirlo de una forma más prolija y entendible :

  # app/controllers/matches_controllers.rb
  def subscribe
    @player = @match.players.create :user => current_user, :team => params[:team].to_i

    flash[:notice] = @player.errors.full_messages.join(", ") if @player.errors.any?

    redirect_to match_path(@match)
  end

Lovely :). Se crea un Player para el Match, se prepara una alerta visual en caso de que haya algún error y luego se redirige a la página del partido. Si ven el antes y el después creo que nadie me va a negar de la mejora :).

Por hoy es todo, hasta la próxima entrega!.

Luego de escribir mucho código uno comienza a mirar a ver que hacen los de al lado, y al mirar se da cuenta de lo mal que uno escribe código cuando comienza con un nuevo lenguaje :). Algo de eso me pasa hoy en día con ¡Falta Uno!.

En el camino de emprolijar las cosas para liberar el código tuve la necesita de poner a refactorizar el código porque hay partes que no son nada bonito. Voy a tratar de plasmar en los siguientes posts algunas lecciones aprendidas junto con el paso a paso del refactoring del código.

Antes que nada y como buena práctica es ideal tener buenos test sobre los que se va a refactorizar ya que es muy fácil romper alguna funcionalidad que se suponía que andaba. Si bien para los ejemplos acá voy a ignorar esta buena práctica, no se los aconsejo :).

Vamos a empezar con el método create de controlador Matches, encargado de crear un nuevo partido :

  def create
    @match = current_user.matches.create(params[:match])
    if @match.errors.empty?
      Player.create :match => @match, :user => current_user

      @notifications = @match.owner.friends.select {|f| f.notify_new_matches? }.collect(&:email)
      Emailer.deliver_match_created(@match, @notifications) if @notifications.any?

      flash[:success] = "El partido fue creado."
      redirect_to matches_path
    else
      render :action => 'new'
    end
  rescue
    render :action => 'new'
  end

Asusta un poco, ¿no?. Para entender el problema, veamos primero como que involucra crear un nuevo partido :

• Crear el partido
• Anotar al usuario que creó el partido en uno de los equipos
• Notificar a todos los amigos que se creó un nuevo partido

Dado esto, pasemos a analizar los problemas. La primer pregunta que habría que hacerse es : ¿Quién es el responsable de anotar al jugador?, ¿Tiene sentido que un usuario pueda crear un partido sin que esté anotado?. En el contexto nuestro, la respuesta a la última pregunta es «No», solo porque así lo defino yo :). Entonces podemos responder a la primer pregunta : El Match debería ser el responsable de anotar al dueño del partido cuando es creado.

Para lograr esto último lo que debemos hacer es mover la lógica de creación del Player dentro de un callback en el modelo Match, quedando algo así :

class Match  self, :user => owner
  end
end

De esta forma nos aseguramos que se cumpla lo pedido sin correr el riesgo de que se puedan crear matches sin que el owner este anotado inicialmente. También ganaríamos en el caso de que exista alguna otra forma de crear un partido, por ejemplo si agregáramos la posibilidad de crear un partido enviando un email, no tendríamos la necesidad de repetir la lógica inicial en dos lugares diferentes.

Y ahora nuestro controlador queda algo cómo :

  def create
    @match = current_user.matches.create(params[:match])
    if @match.errors.empty?
      @notifications = @match.owner.friends.select {|f| f.notify_new_matches? }.collect(&:email)
      Emailer.deliver_match_created(@match, @notifications) if @notifications.any?

      flash[:success] = "El partido fue creado."
      redirect_to matches_path
    else
      render :action => 'new'
    end
  rescue
    render :action => 'new'
  end

Lo siguiente es limpiar un poco lo lógica del modelo. Una cosa que molesta a la vista son las dos llamadas a render, ¿son realmente necesarias? La respuesta en NO. Con el rescue lo que hacemos es que dada una Exception vuelva al formulario de creación, lo mismo que si el partido no es válido, por lo que simplemente podríamos haber usado el método create! que lanza una Exception si el modelo no es válido evitándonos así el molesto if. Quedando ahora :

  def create
    @match = current_user.matches.create!(params[:match])

    @notifications = @match.owner.friends.select {|f| f.notify_new_matches? }.collect(&:email)
    Emailer.deliver_match_created(@match, @notifications) if @notifications.any?

    flash[:success] = "El partido fue creado."
    redirect_to matches_path
  rescue
    render :action => 'new'
  end

Que si lo comparan con la versión inicial es mucho más legible. «Fat model, skinny controllers» es la clave a seguir. Todavía queda por mejorar la forma en que se genera la lista de emails de los amigos que desean recibir la notificación, pero no es algo que me moleste de momento.

En otras entregas seguiré mostrando el resto del refactoring de otro métodos que son aún peores que este :).