How can I simplify this class method?

Question

I have a class that has 50 random people with random scores, stored in an array of hashes:

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

The top method in particular (below) will return the top N people in a string with their score. Being a little newer to ruby, I think there might be a way to make this simpler. Here's the method:

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

For reference, using Ruby 2.3.3

Answer 1

I tried to improve also other things. I miss a bit more context, but responding your question as a theoretical exercise, this is what I would do:

 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

• create an ordered list of people (at the very beginning). That will prevent to order on each call to the top method.
• create a ScoredPerson class to encapsulate the logic of an scored person.

Answer 2

Be Welcoming is our new moto, so here goes...

When you are asking a question please make sure it is a complete and functional question. e.g. "I have a class that has 50 random people with random scores, stored in an array of hashes:[...]The top method in particular will return the top N people in a string with their score."

Please provide us with a class then:

# 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

Now let's talk about what you are actually doing here in Scoreboard#top

Steps:

• Group into a Hash by score
• Sort by Score (in reverse)
• Take the first n groupings
• Map dropping the the scores
• Flatten the Array
• map again into a String of name (score)
• join with commas
• print them all on the same line

This seems like a bit of overkill. The let's try a different solution by finding the cut off instead

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

Now we know the smallest score we will accept so top now just needs those 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 

Now that seems a bit cleaner but People should not be relegated to a dictionary (we have feelings after all) so let's make them a real Object (btw objectifying people is still wrong in real life). Since they are simple creatures having just a first_name and a score a Struct will do just fine.

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

This essentially creates an object that looks like this

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

Now we can create our people like so

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

Now rather than Hash#[] access we have methods for score and name so we can use some Symbol#to_proc sugar (don't worry about this right now but feel free to look into it as it is very ruby idiomatic)

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 

We are almost there now but this "#{person.name} (#{person.score})" seems silly since those are the only attributes anyway so let's just make that the default representation of a Person by defining to_s for our Person

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

Now we have

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

Also since puts returns nil and you can handle the display elsewhere I have removed the puts statement. Since you already know n from outside the call I would suggest something like:

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

Ooh so much cleaner now. Hope you enjoyed the answer and learned something along the way. Full Example

Answer 3

It's probably not a good idea to mix business logic (how to get the best people) with output (how to display a list of people). And you can slightly simplify by combining the sort_by/group_by and using 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)}"