Dado el siguiente código,

¿Cómo refactorizaría esto para que el método search_word tenga acceso a issueid?

Yo diría que cambiar la función search_word para que acepte 3 argumentos o hacer que issueid sea una variable de instancia (@issueid) podría considerarse como un ejemplo de malas prácticas, pero honestamente no puedo encontrar ninguna otra solución. Si no hay otra solución aparte de esto, ¿le importaría explicar la razón por la que no hay otra solución?

Tenga en cuenta que es un modelo Ruby on Rails.

def search_type_of_relation_in_text(issueid, type_of_causality)
    relation_ocurrences = Array.new
    keywords_list = { 
        :C => ['cause', 'causes'],
        :I => ['prevent', 'inhibitors'],
        :P => ['type','supersets'],
        :E => ['effect', 'effects'],
        :R => ['reduce', 'inhibited'],
        :S => ['example', 'subsets'] 
    }[type_of_causality.to_sym]  

    for keyword in keywords_list
        relation_ocurrences + search_word(keyword, relation_type)
    end        

    return relation_ocurrences
end


def search_word(keyword, relation_type)
relation_ocurrences = Array.new

@buffer.search('//p[text()*= "'+keyword+'"]/a').each { |relation|

    relation_suggestion_url   = 'http://en.wikipedia.org'+relation.attributes['href']
    relation_suggestion_title = URI.unescape(relation.attributes['href'].gsub("_" , " ").gsub(/[\w\W]*\/wiki\//, ""))

    if not @current_suggested[relation_type].include?(relation_suggestion_url)
        if @accepted[relation_type].include?(relation_suggestion_url)
            relation_ocurrences << {:title => relation_suggestion_title, :wiki_url => relation_suggestion_url, :causality => type_of_causality, :status => "A", :issue_id => issueid}
        else
            relation_ocurrences << {:title => relation_suggestion_title, :wiki_url => relation_suggestion_url, :causality => type_of_causality, :status => "N", :issue_id => issueid}
        end

    end
} 

end
0
dLobatog 13 ene. 2012 a las 21:39

1 respuesta

La mejor respuesta

Si necesita contexto adicional, páselo como un argumento adicional. Así es como se supone que funciona.

Configurar variables de instancia de tipo @ para pasar contexto es una mala forma, como ha identificado.

Hay una serie de convenciones de Ruby que parece no conocer:

  • En lugar de Array.new simplemente use [ ], y en lugar de Hash.new use { }.
  • Use una instrucción case o una constante en lugar de definir un Hash y luego recuperar solo uno de los elementos, descartando el resto.
  • Evite usar return a menos que sea estrictamente necesario, ya que la última operación siempre se devuelve de forma predeterminada.
  • Utilice array.each do |item| en lugar de for item in array
  • Use do ... end en lugar de { ... } para bloques de varias líneas, donde la versión de llaves se reserva generalmente para frases breves. Evita confusiones con declaraciones hash.
  • Intente evitar la duplicación de grandes fragmentos de código cuando las diferencias sean menores. Por ejemplo, declare una variable temporal, manipúlela condicionalmente y luego almacénela en lugar de definir múltiples variables independientes.

Con eso en mente, aquí hay una reelaboración:

KEYWORDS = { 
    :C => ['cause', 'causes'],
    :I => ['prevent', 'inhibitors'],
    :P => ['type','supersets'],
    :E => ['effect', 'effects'],
    :R => ['reduce', 'inhibited'],
    :S => ['example', 'subsets'] 
}

def search_type_of_relation_in_text(issue_id, type_of_causality)
  KEYWORDS[type_of_causality.to_sym].collect do |keyword|
    search_word(keyword, relation_type, issue_id)
  end
end

def search_word(keyword, relation_type, issue_id)
  relation_occurrences = [ ]

  @buffer.search(%Q{//p[text()*= "#{keyword}'"]/a}).each do |relation|
    relation_suggestion_url = "http://en.wikipedia.org#{relation.attributes['href']}"
    relation_suggestion_title = URI.unescape(relation.attributes['href'].gsub("_" , " ").gsub(/[\w\W]*\/wiki\//, ""))

    if (!@current_suggested[relation_type].include?(relation_suggestion_url))
      occurrence = {
        :title => relation_suggestion_title,
        :wiki_url => relation_suggestion_url,
        :causality => type_of_causality,
        :issue_id => issue_id
      }

      occurrence[:status] =
        if (@accepted[relation_type].include?(relation_suggestion_url))
          'A'
        else
          'N'
        end

      relation_ocurrences << occurrence
    end
  end 

  relation_occurrences
end
5
tadman 13 ene. 2012 a las 22:00
Honestamente, leí en libros como Clean code o incluso en la guía de estilo del kernel que las funciones de 3 argumentos son algo que se debe evitar, aún así, felicitaciones por los excelentes consejos que desconocía :) ¡Eres genial!
 – 
dLobatog
13 ene. 2012 a las 22:33
1
No hay reglas estrictas y rápidas sobre cuántos argumentos son aceptables, aparte de que no debería tener "demasiados", sea lo que sea que eso signifique. Tres es razonable. A veces, cinco está bien siempre que la alternativa sea aún menos atractiva. Sin embargo, quince, quince siempre es una mala idea. En ese momento, use un Hash para transportar sus argumentos.
 – 
tadman
14 ene. 2012 a las 00:35