Ranking lines of a file according to number of occurrences

I recently dived into Ruby, so I want to improve this code. Is this good enough?

def ranksystem
logfile.each do |value|
value.gsub!(/[\n]+/, "")
end
logfile.delete("")

logcount = Hash.new(0)

logfile.each do |v|
logcount[v] += 1
end

logarray = logcount.sort_by {|k,v| v}.reverse

finalreturn = ""

logarray.each_with_index do |value, index|
finalreturn = finalreturn + "Rank #{index+1} : #{value[1]} (#{value[0]})\n"
end

finalreturn

end


This code reads each line in log file like this:

apple
orange
apple
apple
apple
apple
orange
orange
banana
banana


and converts like this:

Rank 1 : 4 (apple)
Rank 2 : 3 (orange)
Rank 3 : 2 (banana)


"Cool" enough? It seems fine, if procedural. Depends what you mean by cool.

I might combine a few steps. I'd also refactor and not put everything into one method, since each separate "chunk" does very distinct, and different, things. Separation makes testing easier.

The below isn't necessarily any better (and the chunk that creates the report may actually be worse--I'm not sure; it'd be less efficient), but it provides some alternate avenues to explore further.

def get_log_lines name
logfile.collect { |l| l.gsub(/\n+/, "")}.reject { |l| l == "" }
end

def ranksystem
loglines = get_log_lines "some_logfile.log"
logcount = Hash[loglines.group_by {|l| l}.collect {|k, v| [v.size, k]}]
i = 0
logcount.keys.sort.reverse.collect { |n| i += 1; "Rank #{i}: #{n} (#{logcount[n]})" }.join("\n")
end


Your function would be better if it accepted an argument for the filename.

I think that you are on the right track with logcount = Hash.new(0) and logcount[v] += 1. However, you would be better off instantiating logcount before starting to process the file. That way, logcount is your only data structure. You don't have to construct an array using IO#readlines, and you can discard empty lines on the fly.

The block passed to sort_by could be smarter. The parameters could be more meaningfully named. Sort by -count instead of count to get the list in descending order without having to call reverse.

To build the result string, use Array#join.

def ranksystem(file)
logcount = Hash.new(0)

File.new(file).each_line do |line|
line.chomp!
logcount[line] += 1 unless line.empty?
end

# Note: .each_with_index.map requires Ruby >= 1.8.7
# (see http://stackoverflow.com/a/4697573)
logcount.to_a
.sort_by { |line, count| -count }
.each_with_index
.map { |value, index| "Rank #{index+1} : #{value[1]} (#{value[0]})" }
.join("\n")
end