Yurii Rashkovskii's Blog

This article is a result of yesterday’s code review session I have done.

I have found that I don’t like the following code (well, a code like this):

def topmost_matches(str,limit=3)
  matches = []
  str.each(' ') do |word|
    word = word.strip.downcase
    matches << word if Entity.find_by_name(word)
  end
  return matches[0, limit] if matches.size > limit
  matches
end

Here is what we can do with this code to make it better:

Well, first of all, let’s simplify last two lines. Since if even matches.size is less than limit, matches[0,limit] will return the whole matches array. This means that we don’t need any kind of explicit condition:

  matches[0,limit]

Then, I really can’t say that the rest of the code is easy to read and understand. It is not readable, it is rather interpretable. And here we can use some Ruby power to do this stuff meach easier and nicer:

 str.split(/\s+/).         # here we split str by whitespace characters
      collect(&:downcase). # then we downcase each word
      select{|word| Entity.find_by_name(word)}[0,limit] 
                           # ...and finally we select that words
                           # that are found within Entities names

Also, it is pretty easy to add even a more complex functionality to this single functional “query”. For example, we also may want to sort results according Entities ratings, descending. No problem!

 str.split(/\s+/). 
      collect(&:downcase).
      select{|word| Entity.find_by_name(word)}.
      collect{|word| Entity.find_by_name(word)}. # collect respective Entities
      sort_by(&:rating).reverse.                 # sort them by rating, descending
      collect(&:name)[0,limit]                   # collect their names, and return topmost 
                                                 # of them

Of course, the above code is still a heaven for code reviewer, but it is out of this article’s scope.

Update: According to improvements proposed in comments, it could be even that easy:

 str.split(/\s+/). 
      collect(&:downcase).
      collect{|word| Entity.find_by_name(word)}.compact.
      sort_by(&:rating).reverse.               
      collect(&:name).first(limit)