Refactor ruby on rails model

Question

Given the following code,

How would you refactor this so that the method search_word has access to issueid?

I would say that changing the function search_word so it accepts 3 arguments or making issueid an instance variable (@issueid) could be considered as an example of bad practices, but honestly I cannot find any other solution. If there's no solution aside from this, would you mind explaining the reason why there's no other solution?

Please bear in mind that it is a Ruby on Rails model.

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

Answer 1

If you need additional context, pass it through as an additional argument. That's how it's supposed to work.

Setting @-type instance variables to pass context is bad form as you've identified.

There's a number of Ruby conventions you seem to be unaware of:

• Instead of Array.new just use [ ], and instead of Hash.new use { }.
• Use a case statement or a constant instead of defining a Hash and then retrieving only one of the elements, discarding the remainder.
• Avoid using return unless strictly necessary, as the last operation is always returned by default.
• Use array.each do |item| instead of for item in array
• Use do ... end instead of { ... } for multi-line blocks, where the curly brace version is generally reserved for one-liners. Avoids confusion with hash declarations.
• Try and avoid duplicating large chunks of code when the differences are minor. For instance, declare a temporary variable, conditionally manipulate it, then store it instead of defining multiple independent variables.

With that in mind, here's a reworking of it:

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