3
\$\begingroup\$

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

2 Answers 2

3
\$\begingroup\$

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}
\$\endgroup\$
6
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 18, 2014 at 7:20
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 18, 2014 at 8:57
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 18, 2014 at 23:37
  • 1
    \$\begingroup\$ Instead of .sort{|(a, _), (b, _)| -(a <=> b)}.first(10) I would use .sort_by(&:first)}.last(10).reverse. \$\endgroup\$
    – Nakilon
    Jan 19, 2014 at 4:57
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 19, 2014 at 17:19
1
\$\begingroup\$

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
\$\endgroup\$
4
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 23, 2014 at 7:05
  • \$\begingroup\$ Oh, you are right, you want a hash as output. Activesupport has an abstraction for that task, are by chance using it? (i.e. Rails). \$\endgroup\$
    – tokland
    Jan 23, 2014 at 8:34
  • \$\begingroup\$ Nope, this is a console app. \$\endgroup\$ Jan 23, 2014 at 12:21
  • \$\begingroup\$ Also, since people (ie. you) seem to still be interested, I bumped the version of the code in the post with what I'm currently using (updated with suggestions from you) \$\endgroup\$ Jan 23, 2014 at 12:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.