current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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:

Since people are still replying, here is the current actual state of play (Note I've removed the intermediate steps now since this post was getting a bit long and dry. Feel free to check the edit history for previous steps this code has taken):

def process_template(template)
  # snip...

  feedback 'Fetching data...'
  data = fetch_data(@config[company.to_s])
  @data = select_data(data)

  feedback 'Generating output...'
  generate(template, output, @config[company.to_s], @data)
end

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

  # False positive
  #noinspection RubyHashKeysTypesInspection
  Hash[provider.data.sort_by {|key, _| key }.last(10).reverse]
end

def select_data(data)
  options = data.keys
  chosen = options.first(2)

  # #timesheet_menu will display a CUI menu to the user to select options from
  until @highline.choose{|menu| timesheet_menu options, chosen, menu} == :done
  end

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

# Locals are available to ERB and used in the templates
#noinspection RubyUnusedLocalVariable
def generate(template, output_file, company, data)
  File.write(output_file, ERB.new(File.read(template), nil, '-').result(binding))
end

private

def timesheet_menu(options, chosen, menu)
  menu.prompt = 'Please select which timesheets to use:'
  options.sort { |a, b| -(a <=> b) }.each do |k|
    menu.choice(k + (chosen.include?(k) ? ' (*)' : '')) do
      if chosen.include? k
        chosen.delete k
      else
        chosen.push k
      end
    end
  end
  menu.choice(:done)
end
share|improve this question
3  
I did an edit based on your "final code", though I confess I'm not familiar with that term. –  Cary Swoveland Jan 18 '14 at 20:27
    
@CarySwoveland: Fixed that too :) –  Matthew Scharley Jan 19 '14 at 4:50

2 Answers 2

up vote 3 down vote accepted

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}
share|improve this answer
    
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. –  Matthew Scharley Jan 18 '14 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. –  Matthew Scharley Jan 18 '14 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. –  Matthew Scharley Jan 18 '14 at 23:37
1  
Instead of .sort{|(a, _), (b, _)| -(a <=> b)}.first(10) I would use .sort_by(&:first)}.last(10).reverse. –  Nakilon Jan 19 '14 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. –  Cary Swoveland Jan 19 '14 at 17:19
up vote 1 down vote

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
share|improve this answer
    
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. –  Matthew Scharley Jan 23 '14 at 7:05
    
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). –  tokland Jan 23 '14 at 8:34
    
Nope, this is a console app. –  Matthew Scharley Jan 23 '14 at 12:21
    
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) –  Matthew Scharley Jan 23 '14 at 12:36

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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