Code re-factoring - should I move code to separate methods?

Question

I have a controller comparing parsed documents with databases of key words. Typical snippets are encapsulated loops looping through the parsed document, the database of key words counting the number a match and building an array of matching words. Here is an example :

@key_words = KeyWords.all
@count = 0
@array_of_matching_words = []
@parsed_document.each do |a|
@key_words.each do |word|
  if a =~ /#{key_word.word}/
    @count = @count+1
    @array_of_matching_words = @array_of_matching_words.push "#{key_word.word}"
  end
end

Both instantiated variable @count and @array_of_matching_words are passed to a view. I have accumulated numerous of those snippets for the other databases of words and I am trying to re-factor the code by moving some of this code into separate methods.

One of the option I have considered is to move the code to a private method in the controller

def get_the_number_and_the_list_of_matching_words
  @key_words = KeyWords.all
  @count = 0
  @array_of_matching_words = []
  @parsed_document.each do |a|
  @key_words.each do |word|
    if a =~ /#{key_word.word}/
      @count = @count+1
      @array_of_matching_words = @array_of_matching_words.push "#{key_word.word}"
    end
  end
end

I then just leave

get_the_number_and_the_list_of_matching_words

in my action controller and the view will get both @count and @array_of_matching_words.

I find this coding "style" terrible. The method "get_the_number_and_the_list_of_matching_words" appears out of the blue, it does not instantiate anything, no parameter is passed, both instantiated variable (@count, @array_of_matching_words) are not visible (they are burried at the bottom of my .rb file in the private methods section). Is this really a good way to re-factor the code or is there other option? Does it make sense to move code to a method when the method is neither an instance method nor a method requiring to pass a parameter and returning new variables?

These sorts of questions are a better fit on the Code Review site. — tadman

tadman tadman · Accepted Answer · 2017-06-19T20:33:29

A method name like get_the_number_and_the_list_of_matching_words is preposterously long, and if it needs to be that long to convey what it's doing then your method is overly ambitious by design. Break it up into smaller chunks.

Your example method sets three different instance variables, depends on one which isn't passed in as an argument, it's just presumed to magically be there, and has no clear objective other than to define @array_of_matching_words.

Generally including things like the type of something in the variable name is extraneous information. @matching_words is vastly preferable here.

What you should do is break out the logic that does the comparison between @key_words and @parsed_document where both of those are passed in as parameters. Rails encourages putting such methods into "concerns" or, where they're applicable to a particular model, in there if that's a good fit.

Whatever KeyWords is could be extended to produce the desired output, for example, making your code look a lot more like this:

def prepare_key_words_matching
  @key_words = KeyWords.all

  @matching_words = KeyWords.matching(@parsed_document)
end

Where you could then use @matching_words.length instead of having a separate @count variable.

Code re-factoring - should I move code to separate methods?

2 Answers