Parse a document and returning uniqueness and total number occurrences

Question

I am parsing a document and returning (1) uniqueness, and (2) total number occurrence.

I believe my code can still be subjected to review. I would like a review of my code as I have even made it too tightly coupled, and some Dependency Injection would be a nice to have.

How can I make this code better?

links.log (input file)

/help_page/1 126.318.035.038
/help_page/1 126.318.035.038
/contact 184.123.665.067
/home 184.123.665.067
/home/2 444.701.448.104
/help_page/1 929.398.951.889
/index 444.701.448.104
/help_page/1 722.247.931.582

log_processor.rb

#!/usr/bin/ruby
# frozen_string_literal: true

class LogProcessor
  def initialize(file_name)
    self.log_file = File.read(file_name)
    self.log_stats = {}
    process_log
  end

  def process_log
    log_file.each_line do |line|
      path, ip = line.split(' ')
      log_stats.key?(path) ? log_stats[path] << ip : log_stats[path] = [ip]
    end
  end

  def page_views
    page_views = log_stats.inject({}) { |hash, (path, ips)| hash.merge(path => ips.count) }
    page_views.sort_by { |_path, count| count }.reverse
  end

  def unique_page_views
    unique_page_views = log_stats.inject({}) { |hash, (path, ips)| hash.merge(path => ips.uniq.count) }
    unique_page_views.sort_by { |_path, count| count }.reverse
  end

  private

  attr_accessor :log_file, :log_stats
end

class FileReadError < StandardError
end

log_aggregator.rb

#!/usr/bin/ruby
# frozen_string_literal: true

require './log_processor'

class LogAggregator
  def initialize(file_path)
    self.log_aggregator = LogProcessor.new(file_path)
  end

  def most_page_views
    page_views = log_aggregator.page_views
    page_views.map { |path, ip_count| "#{path} - #{ip_count} visits" }.join("\n")
  end

  def most_unique_page_views
    unique_views = log_aggregator.unique_page_views
    unique_views.map { |path, uniq_ips| "#{path} - #{uniq_ips} unique views" }.join("\n")
  end

  def aggregator
    ['MOST PAGE VIEWS:', most_page_views,
     'MOST UNIQUE PAGE VIEWS:', most_unique_page_views].join("\n")
  end

  def print_aggregate
    puts aggregator
  end

  private

  attr_accessor :log_aggregator
end

parser.rb

#!/usr/bin/ruby
# frozen_string_literal: true

require './log_aggregator'

class Parser
  def initialize(file_name)
    @file_name = file_name
  end

  def call
    return expected_command_hint unless file_name_correct?

    parser_result
  end

  private

  attr_reader :file_name

  def parser_result
    LogAggregator.new(file_name).print_aggregate
  end

  def file_name_correct?
    file_name == 'webserver.log'
  end

  def expected_command_hint
    puts 'The right usage is: ./parser.rb webserver.log'
  end
end

Parser.new(ARGV.first).call

running the code:

ruby ./parser.rb links.log

Answer 1

How can I make this code better?

By making it simpler and shorter. The current solution creates many unnecessary abstractions.

Idiomatic ruby should embrace the built-in methods of Hash and Array. They are very often enough to solve your problems, especially data transformation problem like this one.

So, assume you have read the lines of the file into an variable lines which now holds an array of arrays by doing something like:

lines = File.read("myfile").split("\n").map { |x| x.split(' ') }

You simply want the pages sorted by count, and by unique count. This is essentially a couple 1-liners:

views = lines.reduce({}) { |m, (k, v)| m.merge(k => (m[k] || []) + [v]) }.to_a
view_cnts = views.map { |k, v| [k, v.count] }.sort_by { |x| -x[1] }
uniq_view_cnts = views.map { |k, v| [k, v.uniq.count] }.sort_by { |x| -x[1] }

If the code is a bit too dense for you, you can use multi-line do... end for the blocks and names such as page / ip instead of k / v:

view_cnts = views.map do |page, ips|
  [page, ips.count] 
end.sort_by { |(page, cnt)| -cnt }

That's a matter of taste and comfort with ruby. And if you want a hash as output, just call to_h at the end.

But creating class abstractions to solve a straightforward data transformation problem is classic over-engineering.

Answer 2

At least one place I can see room for improvement.

 def process_log
   log_file.each_line do |line|
     path, ip = line.split(' ')
     log_stats.key?(path) ? log_stats[path] << ip : log_stats[path] = [ip]
   end
 end

Rather, let's default log_stats[path] to an empty list if it doesn't yet exist, using ||=. Then we know we can push ip onto that list.

  def process_log
    log_file.each_line do |line|
      path, ip = line.split(' ')
      log_stats[path] ||= []
      log_stats[path] << ip 
    end
  end

But as process_log is never called anywhere else, it's reasonable to use each_with_object to generate the hash for log_stats.

  def initialize(file_name)
    @log_file = File.read(file_name)
    @log_stats = @log_file.lines.each_with_object({}) do |line, h|
      path, ip = line.split(' ', 2)
      h[path] ||= []
      h[path] << ip
    end
  end

It's questionable what benefit there is to having private attr_accessor methods for LogProcessor, unless you just really don't like typing @ symbols.