Ruby Code Critique

Question

Я новичок в Ruby (начал 4 дня назад и эй рифмуется!) И решил написать простой маленький инструмент для развлечения / обучения.Следующий код был результатом.Это прекрасно работает, но я был бы очень признателен критике со стороны более опытных разработчиков Ruby.Я ищу комментарии по стилю, многословию и любым другим вопросам.советы / хитрости.

Кстати, мне действительно интересно, как я могу переписать лямбда-рекурсию, чтобы она была более элегантной.(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'] }

• Я думаю, что ситуация с вложенными лямбда-мишенями выглядит немного беспорядочно и выглядит созревшей для того, чтобы ее можно было разделить на несколько методов.Я также не стал бы пытаться распутать это без некоторых тестов.

• Попробуйте использовать более описательные имена переменных / методов.Например, 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

Если кто-то еще захочет использовать этот метод, ему придется запомнить сокращения, которые вы использовали в имени метода, и порядок, в котором аргументы должныпоявляются в, что не весело.Более похожая на ruby ​​альтернатива была бы:

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

Или, с новым синтаксисом Hash в Ruby 1.9, это может бытьеще более приятный:

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

Но в любом случае, вы чувствуете себя намного лучше, чем я с Руби после четырех дней.Если вы планируете строить этот фрагмент кода в будущем, я бы определенно попытался добавить несколько тестов, прежде чем начать рефакторинг.Посмотрите на RSpec .Хорошим введением в это будет глава 1 Ruby Best Practices (бесплатный PDF).

Наслаждайтесь Ruby!

Answer 2

Я не полностью прочитал ваш код, но

char_skills.each_pair do |outer_skill_name, *|

можно заменить на

char_skills.each_key do |outer_skill_name|