Ruby function to fetch, filter, and generate data

Question

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
    break if :done == choose {|menu| timesheet_menu valid_keys, menu}
  end

  puts 'Generating output...'
  # Used as a result of the ERB binding
  #noinspection RubyUnusedLocalVariable
  data = @data.select {|k, _| valid_keys.include? k}
  File.write(output_file, ERB.new(File.read(template), nil, '-').result(binding))
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
    break if :done == choose {|menu| timesheet_menu data, valid_keys, menu}
  end

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

Answer 1

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
      break if :done == choose {|menu| timesheet_menu data, data.map(&:first), menu}
    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
   break if (keys_chosen = choose {|menu| timesheet_menu data, data.keys.first(2), menu})
 end

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

Answer 2

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)
  until (choose { |menu| timesheet_menu(data, valid_keys, menu) }) == :done
  end
  data.keys & valid_keys
end