Ruby Code Critique

Question

我是一个Ruby新手(4天前开始,嘿韵!)并决定编写一个简单的小工具,用于娱乐/学习.以下代码是结果.它工作正常,但我真的很感谢来自一些更有经验的Ruby开发人员的批评.我正在寻找关于风格,冗长和任何misc的评论.提示与技巧.

顺便说一句,我真的很想知道如何重写lambda递归更优雅.(ctrl-f代表'lambda recursion')

注意:这不是Ruby On Rails项目.这是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'

这是'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

我注意到的一些事情:

• 使用块时,使用{ ... }单行和do ... end多行是惯用的.你倾向于混合它们.

• if results.include?(name) == false 会读得更好 unless results.include? name

• if (details.include? 'Prerequisites') && (details['Prerequisites'] != nil)if details['Prerequisites']如果你不明白为什么,可以缩短为Ruby中的"真实性".同样,results.reject {|x| (spec[x].include? 'Private') && (spec[x]['Private'] == true)}可以成为results.reject { |x| spec[x]['Private'] }

• 我认为嵌套的lambda情况有点乱,看起来很成熟,可以提取成几种方法.不过,如果没有一些测试,我也不会开始试图解开它.

• 尝试使用更具描述性的变量/方法名称.例如,calc_remaining_sp可以用简单的方式更好地表示remaining_skillpoints.

通常,更多行更干净的代码比更少密码的代码行更可取.例:

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" }

这可以改写为:

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" }

最后,当你开始编写更多方法时,尝试给它们一个容易使用的api.例如,剩余的技能点方法:

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 remaining_skillpoints(options)
  skillpoints = BASE_SKILLPOINTS.map { |x| x * options[:rank] }
  difference  = skillpoints[options[:to]] - skillpoints[options[:from]]
  [difference, 0].max
end

这会让另一个Rubyist做类似的事情:

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

或者,使用Ruby 1.9中的新Hash语法,它可能更令人愉快:

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

但无论如何,在四天之后,你做得比我在Ruby上好多了.如果这段代码是您计划在将来构建的内容,我肯定会在您开始重构之前尝试添加一些测试.看看RSpec.对它的一个很好的介绍将是Ruby Best Practices(免费PDF)的第1章.

享受Ruby吧!