Ruby Code Critique

Question

I'm a Ruby newbie (started 4 days ago and hey it rhymes!) and decided to code a simple little tool for fun/learning. The following code was the result. It works fine, but I would really appreciate critique from some more experienced Ruby developers. I'm looking for comments on style, verbosity, and any misc. tips/tricks.

By the way, I'm really interested in how I can rewrite the lambda recursion to be more elegant. (ctrl-f for 'lambda recursion')

Note: This is not a Ruby On Rails project. It's a little tool for Eve Online.

require 'rubygems'
require 'reve'
require 'yaml'

BASE_SKILLPOINTS = [0, 250, 1414, 8000, 45255, 256000]

def calc_remaining_sp(rank, from_level, to_level)
  sp = BASE_SKILLPOINTS.map { |x| x * rank }
  [sp[to_level] - sp[from_level], 0].max
end

def summarize_skills(user_id, api_key)
  spec = YAML::load(File.open('ukc_skillsets.yaml'))

  api = Reve::API.new user_id, api_key

  #skill id => general skill info
  skills_list = Hash[api.skill_tree.map { |x| [x.type_id, x] }]


  api.characters.each { |char|
    char_sheet = api.character_sheet :characterID => char.id

    puts ""
    puts "Character - #{char.name}"
    puts "-------------------------------------------------"

    char_skills = Hash[skills_list.map { |id, skill| [skill.name, {:level => 0, :remaining_sp => [], :info => skill}] }]

    char_sheet.skills.each do |skill|
      skill_name = skills_list[skill.id].name
      char_skills[skill_name][:level] = skill.level
    end

    #compute the sp needed for each skill / each level
    char_skills.each_pair do |outer_skill_name, *|
      #lambda recursion
      calc_prereq_sp = lambda do |skill_name|
        prereq_remaining_sp = char_skills[skill_name][:info].required_skills.inject(0) do |sum, prereq_skill|
          prereq_skill_name = skills_list[prereq_skill.id].name
          #call the lambda
          calc_prereq_sp.call prereq_skill_name
          sum + char_skills[prereq_skill_name][:remaining_sp][prereq_skill.level]
        end
        current_skill = char_skills[skill_name]
        (0..5).each do |target_level|
          char_skills[skill_name][:remaining_sp][target_level] =
                  (calc_remaining_sp current_skill[:info].rank, current_skill[:level], target_level) +
                          prereq_remaining_sp
        end
      end
      calc_prereq_sp.call outer_skill_name
    end

    results = {}

    spec.each_pair do |skillset_name, *|
      process_skillset =  lambda do |name|

        details = spec[name]
        #puts "#{results} , name = #{name}"
        if results.include?(name) == false
          #puts "#{details['Prerequisites']}"
          remaining_sp = 0
          remaining_sp += details['Prerequisites'].inject(0) { |sp_remaining, prereq|
            process_skillset.call prereq
            sp_remaining + results[prereq][:remaining_sp]
          } if (details.include? 'Prerequisites') && (details['Prerequisites'] != nil)
          details['Required skills'].each_pair { |required_skill, target_level|
            remaining_sp += char_skills[required_skill][:remaining_sp][target_level]
          } if (details.include? 'Required skills') && (details['Required skills'] != nil)
          results[name] = {:remaining_sp => remaining_sp}
        end
      end
      process_skillset.call skillset_name
    end
    results.reject {|x| (spec[x].include? 'Private') && (spec[x]['Private'] == true)}.to_a().sort_by {|x|x[1][:remaining_sp]} \
      .each { |x, y| puts "#{x} = #{y[:remaining_sp]} sp left\n" }
  }
end
#userid, apikey hidden for confidentiality
summarize_skills 'xxxxxxx', 'yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy'

and here's a short snippet of 'ukc_skillsets.yaml'

Basics:
    #private = don't show at the end
    Private: True
    Required skills:
        #skill name: skill level
        Electronics: 4
        Engineering: 4
        Weapon Upgrades: 1
        Energy Systems Operation: 1
Armor Ship:
    Private: True
    Prerequisites:
        - Basics
    Required skills:
        Hull Upgrades: 4
        Mechanic: 4
......

Answer 1

I haven't read through your code fully, but

char_skills.each_pair do |outer_skill_name, *|

can be replaced with

char_skills.each_key do |outer_skill_name|

Answer 2

A few things I noticed:

• When using blocks, it's idiomatic to use { ... } for a single line and do ... end for multiple lines. You tend to mix them up a bit.

• if results.include?(name) == false would read better as unless results.include? name

• if (details.include? 'Prerequisites') && (details['Prerequisites'] != nil) can be shortened to if details['Prerequisites'] Read about "truthiness" in Ruby if you don't understand why. Likewise, results.reject {|x| (spec[x].include? 'Private') && (spec[x]['Private'] == true)} can become results.reject { |x| spec[x]['Private'] }

• I think the nested lambda situation is a bit of a mess, and looks ripe to be extracted into a few methods. I also wouldn't start trying to untangle it without some tests, though.

• Try using more descriptive variable/method names. For example, calc_remaining_sp could be better represented by a simple remaining_skillpoints.

In general, more lines of cleaner code is preferable to fewer lines of denser code. Example:

results.reject {|x| (spec[x].include? 'Private') && (spec[x]['Private'] == true)}.to_a().sort_by {|x|x[1][:remaining_sp]} \
  .each { |x, y| puts "#{x} = #{y[:remaining_sp]} sp left\n" }

This could be rewritten as something like:

results.reject!  { |skill| spec[skill]['Private'] }
results.sort_by! { |skill| skill[1][:remaining_sp] } # sort_by! requires 1.9 - for 1.8 use results = results.sort_by...
results.each     { |x, y| puts "#{x} = #{y[:remaining_sp]} sp left\n" }

Finally, when you do start writing more methods, try to give them an easily usable api. For example, the remaining skillpoints method:

def calc_remaining_sp(rank, from_level, to_level)
  sp = BASE_SKILLPOINTS.map { |x| x * rank }
  [sp[to_level] - sp[from_level], 0].max
end

If somebody else wants to use this method, they're going to have to memorize the abbreviations you used in the method name and the order the arguments are supposed to appear in, which is not fun. A more ruby-like alternative would be:

def remaining_skillpoints(options)
  skillpoints = BASE_SKILLPOINTS.map { |x| x * options[:rank] }
  difference  = skillpoints[options[:to]] - skillpoints[options[:from]]
  [difference, 0].max
end

This would let another Rubyist do something like:

points_needed = remaining_skillpoints :rank => 6, :from => 3, :to => 5

Or, with the new Hash syntax in Ruby 1.9, it can be an even more pleasant:

points_needed = remaining_skillpoints rank: 6, from: 3, to: 5

But anyway, you're doing much better than I was with Ruby after four days. If this piece of code is something you're planning on building on in the future, I would definitely try adding some tests before you start refactoring. Look into RSpec. A good introduction to it would be chapter 1 of Ruby Best Practices (free PDF).

Enjoy Ruby!