Ruby function to fetch, filter, and generate data

I'm currently writing a script and I decided to run cane over it to look for some issues, and the following method was highlighted. I've done my best to cut it back, and at this point I'm inclined to leave it as-is, however cane is still giving it a pretty high rating of 23 (15 is the point it starts complaining) so I wanted to get some more input. Other than perhaps splitting fetching and generating does anyone have any ideas?

def self.generate(template, output_file, company)
puts 'Fetching data...'
@data = Provider::GenericProvider.create_provider(company['type'].to_sym, company).data

valid_keys = @data.keys.sort {|a,b| -(a <=> b)}[0,10]
@data = @data.select {|k, _| valid_keys.include? k}

valid_keys = @data.keys.sort {|a,b| -(a <=> b)}[0,2]

loop do
end

puts 'Generating output...'
# Used as a result of the ERB binding
#noinspection RubyUnusedLocalVariable
data = @data.select {|k, _| valid_keys.include? k}
end


EDIT:

I just split it, and ended up with the below function, which still scores 19 due to the various lambdas used to control sorting and filtering the data despite being pretty easy to follow still:

def self.fetch_data(company)
data = Provider::GenericProvider.create_provider(company['type'].to_sym, company).data

valid_keys = data.keys.sort {|a,b| -(a <=> b)}[0,10]
data = data.select {|k, _| valid_keys.include? k}

valid_keys = data.keys.sort {|a,b| -(a <=> b)}[0,2]

loop do
end

data.select {|k, _| valid_keys.include? k}
end

• I did an edit based on your "final code", though I confess I'm not familiar with that term. Jan 18, 2014 at 20:27
• @CarySwoveland: Fixed that too :) Jan 19, 2014 at 4:50
• I have rolled back revisions that were based on an answer. Please see What to do when someone answers. Sep 10, 2016 at 17:56

Matthew, it appears to me that you are only making use of the two elements of data whose keys are the top two in the sort. Please correct me if I am wrong. If that is the case, I believe your code (after "edit") can be simplified to this:

  def self.fetch_data(company)
data = Provider::GenericProvider.create_provider(company['type'].to_sym, company).data
data = data.sort {|(k1,_),(k2,_)| -(k1<=>k2)}.first(2)
loop do
end
Hash[data]
end


Note that after the sort, data is an array of two 2-tuples, each corresponding to a key-value pair from the original hash. If I misunderstood what you are doing, and you do need to pull out the 10 elements of the hash corresponding to the 10 highest-ranking keys, just change the parameter for first from 2 to 10, then extract the first two elements when you need them (i.e., data.first(2)). You can then retrieve the key values from these 2-tuples or convert data.first(2) to a hash for the return value, as I have done with data.

Edit: Ah, valid_keys is modified by choose. Consider this as a further possibility:

 data = Hash[data.sort{|(a, _), (b, _)| -(a <=> b)}.first(10)]

loop do
end

data.select {|k, _| keys_chosen.include? k}

• The first sort/filter applies filters the data to the latest 10 entries (the keys are dates in ISO format; somewhat irrelevant, but context). valid_keys is initialised to the first two elements initially, choose ... timesheet_menu is a Highline menu that then displays a menu to the user and asks them which they'd like to use which modifies valid_keys as a side effect. Jan 18, 2014 at 7:20
• Thanks! I managed to get it down to a score of 14 which puts it below the normal cutoff for warnings using some of the suggestions here. I've included the final code in a further edit to the question if you're interested. Jan 18, 2014 at 8:57
• Unfortunately, Highline#choose is beyond my control, so I can't implement it this way. The reason for the loop at all is because #choose will only handle a single answer from the user, which toggles one of the options on or off (handled by #timesheet_menu), and then returns the option chosen as well. :done is the option that signifies the user has finished their modifications which drops out of the loop and continues the main logic. See highline.rubyforge.org/doc/classes/HighLine.html#M000011 for more on the Highline gem. Anyway, thanks a lot for the input. Jan 18, 2014 at 23:37
• Instead of .sort{|(a, _), (b, _)| -(a <=> b)}.first(10) I would use .sort_by(&:first)}.last(10).reverse. Jan 19, 2014 at 4:57
• I hadn't considered that, @Nak, (and have tucked it away) but I have a slight preference for what I have. My scorecard has them tied on readability, but the incumbent has the edge on simplicity and (I'm guessing) efficiency. Jan 19, 2014 at 17:19

I've taken the code labeled as "final revision" in your question. Some notes:

• data = something and then data = another_thing. So data holds two different values, that's confusing (use another variable name).

• Spaces between lines: Vertical space is very valuable, don't insert blank lines without a reason (for example, to separate logically different parts of the code).

• sort_by is usually a better option than sort (higher level of abstraction).

• xs[0, 2] -> xs.first(2). More declarative.

• break if :done == choose .... Is that a Yoda-condition? it does not look idiomatic in Ruby.

I'd write:

def self.fetch_data(company)
provider = Provider::GenericProvider.create_provider(company['type'].to_sym, company)
data = Hash[provider.data.sort_by { |key, value| key }.reverse.first(10)]
valid_keys = data.keys.first(2)

• Regarding your return value, is that the same as the select given? In particular, does it return a hash? It looks like it'd just return the keys when I want both. As to #choose... it's taking user input and modifying valid_keys based on it. So, technically no it's not the same arguments. At any rate, as addressed with Cary, choose is provided by Highline so I can't really mess with that too much. Jan 23, 2014 at 7:05