5
\$\begingroup\$

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

def ranksystem
    logfile = IO.readlines("some_logfile.log")
    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)
\$\endgroup\$
4
\$\begingroup\$

"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 = IO.readlines(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
\$\endgroup\$
2
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.