如何简化此类方法？答案

【问题标题】：How can I simplify this class method?如何简化此类方法？
【发布时间】：2018-10-08 14:27:42
【问题描述】：

我有一个班级，有 50 个随机分数的随机人，存储在哈希数组中：

def initialize
    # adding fake names and numbers
    require 'faker'
    @people = []
    (1..50).each do
      @people << { name: Faker::Name.first_name, score: Faker::Number.between(1, 1000).to_i }
    end
end

top 方法（如下）将返回字符串中排名前 N 的人及其分数。作为 ruby 的新人，我认为可能有一种方法可以让这变得更简单。方法如下：

def top(number=10)
    top_people = @people.group_by { |person| person[:score] }
                        .sort_by { |key, value| -key }     # largest -> smallest
                        .first(number)
                        .map(&:last)
                        .flatten
                        .map { |person| "#{person[:name]} (#{person[:score]})" }
                        .join(", ")
    puts "The top #{number} are here: #{top_people}"
end

供参考，使用 Ruby 2.3.3

【问题讨论】：

A> 为此编写自动化测试，因此更改代码更安全。 B> "#{person[:name]} (#{person[:score]})" 可以是 '%{name} (%{score})' % person。并且 C> 不要编写连续语句。在单独的行中使用top_people = top_people.sort_by...。
@Phlip 重复top_people = ... 真的很惯用吗？我会认为代码有异味，因为所有中间行都不会是top_people，而是sorted_groups、top_groups 等。只有在扁平化之后，你才真正拥有top_people
我正在维护一个项目，其中有人将所有内容都塞进一个语句中，没有中间变量和大量换行点\n。像你的。请不要让我开始！ C-;
@Phlip 我指的是局部变量名称。请参阅我的更新答案。
(1..50).each 可能是50.times

标签： ruby class oop

【解决方案1】：

将业务逻辑（如何获得最优秀的人员）与输出（如何显示人员列表）混合在一起可能不是一个好主意。您可以通过结合 sort_by/group_by 并使用 flat_map 来稍微简化：

class MyClass
  def top number=10
    top_groups = @people.group_by { |person| person[:score] }.max_by(number, &:first)
    top_groups.flat_map(&:last)
  end

  def self.show_people people
    people.map { |person| "%{name} (%{score})" % person }.join(", ")
  end
end

some_class = MyClass.new
top_people = some_class.top 10
puts "The top #{top_people.size} people are #{MyClass.show_people(top_people)}"

【讨论】：

你可以更进一步，把人变成自己的对象，知道如何展示自己。
考虑使用max_by(top_number) {...}。

【解决方案2】：

Be Welcoming 是我们的新摩托车，所以这里...

当您提出问题时，请确保它是一个完整且实用的问题。例如“我有一个类有 50 个随机分数的随机人，存储在一个哈希数组中：[...]特别是 top 方法将返回一个字符串中的前 N 个人及其分数。” em>

那么请给我们提供一个类：

# require your dependencies outside of the class 
# this is where they will end up anyway and it makes it easier for us
# to find them
require 'faker'

class Scoreboard
   def initialize
     # adding fake names and numbers

     @people = []
     (1..50).each do
       @people << { name: Faker::Name.first_name, 
                    score: Faker::Number.between(1, 1000).to_i }
     end
  end
  def top(number=10)
    top_people = @people.group_by { |person| person[:score] }
                    .sort_by { |key, value| -key }     # largest -> smallest
                    .first(number)
                    .map(&:last)
                    .flatten
                    .map { |person| "#{person[:name]} (#{person[:score]})" }
                    .join(", ")
    puts "The top #{number} are here: #{top_people}"
  end
end

现在让我们来谈谈你在Scoreboard#top这里实际在做什么

步骤：

按分数分组为Hash
按分数排序（反向）
取前 n 个分组
地图降低分数
展平Array
再次映射成名称字符串（分数）
用逗号加入
将它们全部打印在同一行上

这似乎有点矫枉过正。让我们通过寻找截断来尝试不同的解决方案

def cut(n=10) 
  @people.map {|p| p[:score]}.uniq.sort.last(n).first
end

现在我们知道了我们将接受的最小分数，所以 top 现在只需要那些 people

def top(n=10)
  top_people = @people.select {|p| p[:score] >= cut(n) }
               .sort_by {|p| -p[:score]}
               .map { |person| "#{person[:name]} (#{person[:score]})" }
  puts "The top #{n} are here: #{top_people.join(',')}"
end

现在看起来有点干净了，但 People 不应该被归为字典（毕竟我们有感情）所以让我们把它们变成一个真实的 Object （顺便说一句，客观化人们在现实生活中仍然是错误的）。因为它们是简单的生物，只有 first_name 和 score Struct 就可以了。

Person = Struct.new(:name, :score)

这实际上创建了一个看起来像这样的对象

class Person 
  attr_accessor :name, :score 
  def initialize(name,score)
    @name = name
    @score = score
  end
end

现在我们可以像这样创造我们的人了

def initialize
  # we will use Enumerable#map rather than initializing an Array
  # and pushing into it
  @people = (1..50).map do 
    Person.new(Faker::Name.first_name, 
        Faker::Number.between(1, 1000).to_i)
  end
end

现在，而不是 Hash#[] 访问，我们有 score 和 name 的方法，所以我们可以使用一些 Symbol#to_proc 糖（现在不要担心这个，但请随时调查它，因为它非常红宝石惯用语）

def cut(n=10) 
  @people.map(&:score).uniq.sort.last(n).first
end
def top(n=10)
 top_people = @people.select {|p| p.score >= cut(n) }
              .sort_by(&:score).reverse
              .map { |person| "#{person.name} (#{person.score})" }
 puts "The top #{n} are here: #{top_people.join(',')}"
end

我们现在差不多了，但是这个"#{person.name} (#{person.score})" 看起来很傻，因为无论如何这些都是唯一的属性，所以让我们通过为我们的Person 定义to_s 来使其成为Person 的默认表示

Person = Struct.new(:name, :score) do 
   def to_s
      "#{name} (#{score})"
   end
end

现在我们有

def top(n=10)
  top_people = @people.select {|p| p.score >= cut(n) }
               .sort_by(&:score).reverse
end

此外，由于puts 返回nil，并且您可以在其他地方处理显示，因此我删除了puts 语句。由于您已经在通话之外知道n，我建议您这样做：

n = 12
puts "The top #{n} people are:" 
puts Scoreboard.new.top(n)

哦，现在干净多了。希望您喜欢这个答案并在此过程中学到了一些东西。 Full Example

【讨论】：

嘿-谢谢！刚从假期回来，我确实从这篇文章中学到了很多 :) 我确实做了 2 处细微的修改，因为我认为你的答案是针对得分独特的人而不是得分相同的人：1）删除了@987654354来自cut 内部的@ 方法，以及2) 将.first(n) 添加到top_people 以获得每个人的真正前n 个。我还在top 的开头添加了n = @people.length unless @people.length > n，以防用户要求的前n 大于人的大小
@Mike 删除 uniq 是有道理的，尽管它现在与您的原始实现不同。至于first 评论，如果人们得分相同，您如何确定最高。例如，有 5 个并列第 7 名的人被从名单中删除。第三个first 和last 已经为您处理大于Array 大小的数字，因此不需要例如[1,2].first(10) #=> [1,2]
我把它想象成一个街机柜记分牌——显示的数量是有硬性限制的（当然通常按另一个参数排序，比如获取的时间，但我现在不打算添加它）。回复：在top 的顶部添加一行 - 你是对的，我只是再次遍历所有内容并且不需要它。我最初在重构之前添加了它，但没有取出它，去度假，然后重构并保留它

【解决方案3】：

我还尝试改进其他方面。我错过了更多背景信息，但是将您的问题作为理论练习来回答，这就是我要做的：

 require 'faker'

 class ScoredPerson
  attr_reader :name
  attr_reader :score

  def initialize
    @name = Faker::Name.first_name
    @score = Faker::Number.between(1, 1000).to_i
  end
end

class TopPeople
  attr_accessor :people

  def initialize
    @people = 50.times.map do
      ScoredPerson.new
    end.sort_by { |p| p.score }.reverse
  end

  def top(number=10)
    people.first(number)
      .map { |p| "#{p.name} (#{p.score})"}.join(", ")
  end
end

创建一个有序人员列表（在开始时）。这将阻止在每次调用 top 方法时进行排序。
创建一个ScoredPerson 类来封装得分人的逻辑。

【讨论】：