Refactoring with simple rules

exercise_1_intent.rb

class Foo
  def something(bar)
    bar.do_something
  end
end

class Account
  def initialize
    @balance = "0"
  end

  def deposit(amount)
    amount.times { @balance.succ! }
  end

  def balance
    @balance.to_i
  end
end

def downcased_email(username, domain)
  [username, domain].map(&:downcase).join "@"
end

def copy_lines_matching(infilename, outfilename, regex)
  lines = File.readlines(File.expand_path(infilename)).select {|l| regex.match l }
  File.open(File.expand_path(outfilename), 'w') do |file|
    lines.each {|l| file << l }
  end
end

exercise_1_intent_answers.rb

class Foo
  def something(bar)
    # send a message to a passed in object. Impossible to know whether it has any side effects!
    bar.do_something
  end
end

class Account
  def initialize
    @balance = "0"
  end

  def deposit(amount)
    # send a message to an internal state-changing object
    # admittedly you would not typically see this kind of an implementation, but I wanted to
    # illustrate an example of state-changing methods on instance variables
    amount.times { @balance.succ! }
  end

  # compute and return a value, easy!
  def balance
    @balance.to_i
  end
end

# compute and return a value
def downcased_email(username, domain)
  [username, domain].map(&:downcase).join "@"
end

# write out to a file
# other possible outcomes could be error messages if the file doesn't exist, or the
# user doesn't have permission to read or write
def copy_lines_matching(infilename, outfilename, regex)
  lines = File.readlines(File.expand_path(infilename)).select {|l| regex.match l }
  File.open(File.expand_path(outfilename), 'w') do |file|
    lines.each {|l| file << l }
  end
end

exercise_2_extractions.rb

# I have not included the User class that powers these examples. Can you implement a
# User class to work with the MyClass#top_users method?


# we start with this...

class MyClass
  def top_users(users)
    users.select {|u| u.registered? && u.registered_before(3.days.ago) }.
      reject {|u| u.role == "admin" }.
      sort_by {|u| %w(email website twitter).index u.referral_stream }.
      first(5)
  end
end

# and extract a couple simple local variables...getting rid of "magic constants"
class MyClass
  def top_users(users)
    admin_role = "admin"
    referral_streams = %w(email website twitter)
    at_most_users = 5
    registration_threshold = 3.days.ago

    users.select {|u| u.registered? && u.registered_before(registration_threshold) }.
      reject {|u| u.role == admin_role }.
      sort_by {|u| referral_streams.index u.referral_stream }.
      first(at_most_users)
  end
end

# now we can do the same with that big functional-style chain
class MyClass
  def top_users(users)
    admin_role = "admin"
    referral_streams = %w(email website twitter)
    at_most_users = 5
    registration_threshold = 3.days.ago

    registered_users = users.select {|u| u.registered? && u.registered_before(registration_threshold) }
    non_admins = registered_users.reject {|u| u.role == admin_role }
    ordered_users = non_admins.sort_by {|u| referral_streams.index u.referral_stream }

    ordered_users.first(at_most_users)
  end
end

# now that we have these local variables, we might choose to extract some of them to methods
# I'll start with the simple local variables first
class MyClass
  def top_users(users)
    registered_users = users.select {|u| u.registered? && u.registered_before(registration_threshold) }
    non_admins = registered_users.reject {|u| u.role == admin_role }
    ordered_users = non_admins.sort_by {|u| referral_streams.index u.referral_stream }

    ordered_users.first(at_most_users)
  end

  def registration_threshold
    3.days.ago
  end

  def admin_role
    "admin"
  end

  def referral_streams
    %w(email website twitter)
  end

  def at_most_users
    5
  end
end


# and now we'll move those other lines into their own methods
class MyClass
  def top_users(users)
    registered_users = recently_registered users
    non_admins = all_non_admins registered_users
    ordered_users = order_by_referral_stream non_admins

    ordered_users.first(at_most_users)
  end

  private

  def recently_registered(users)
    users.select {|u| u.registered? && u.registered_before(registration_threshold) }
  end

  def all_non_admins(users)
    users.reject {|u| u.role == admin_role }
  end

  def order_by_referral_stream(users)
    users.sort_by {|u| referral_streams.index u.referral_stream }
  end

  def registration_threshold
    3.days.ago
  end

  def admin_role
    "admin"
  end

  def referral_streams
    %w(email website twitter)
  end

  def at_most_users
    5
  end
end


# Interestingly, our new class is about 4x the size of the original one, but the
# #top_users method has stayed the same number of lines
# Which is better? It's a matter of opinion... but I prefer a more readable method,
# even if that means there are more lines of code due to decomposed helper methods.
# We didn't add any logic - we just split out the logic that was there into
# well-named boundaries


# Now for some fun, we'll do one more extraction - a bonus Extract Class extraction
# I notice some subtle duplication here... the idea of passing around a list of
# users and filtering based on it. So I'll split those methods up into their own class:
class MyClass

  class UserList < Array
    def recently_registered(treshold)
      self.class.new(select {|u| u.registered? && u.registered_before(threshold) })
    end

    def all_non_role(role)
      self.class.new(reject {|u| u.role == role })
    end

    def order_by_referral_stream(referral_streams)
      self.class.new(sort_by {|u| referral_streams.index u.referral_stream })
    end
  end

  def top_users(users)
    UserList.new(users).recently_registered(registration_threshold).
      all_non_role(admin_role).
      order_by_referral_stream(referral_streams).
      first(at_most_users)
  end

  private

  def registration_treshold
    3.days.ago
  end

  def admin_role
    "admin"
  end

  def referral_streams
    %w(email website twitter)
  end

  def at_most_users
    5
  end
end

# I also find it interesting that the newly-refactored method is structurally similar
# to the original one - it is again a chain of functional-style calls, only this time
# we've encapsulated those functional-style calls into methods with intention-revealing
# names. This makes it a lot easier to understand what the code is doing, and makes it
# easier to change because we only need to locate the one method that needs to change.
# With a few small tweaks, this example could be made even more powerful by introducing
# polymorphism - can you see how to do it?

exercise_3_refactor_this.rb

require 'date'
stuff = File.file?('stuff.txt') ? File.readlines('stuff.txt').map(&:chomp) : []

if ARGV[0] == 'a'
  new_stuff = ARGV[1..-1]
  new_stuff << Date.today
  stuff << new_stuff.join(':')
elsif ARGV[0] == 'r'
  split_stuff = stuff.map {|s| s.split(':') }
  grouped_stuff = split_stuff.group_by {|s| s.last }
  grouped_stuff.each {|k, v| grouped_stuff[k] = v.group_by {|s| s.first } }
  sorted_keys = grouped_stuff.keys.sort
  sorted_keys.each do |key|
    parsed = Date.parse(ARGV.last) rescue nil
    next if parsed && parsed != Date.parse(key)

    puts key
    group = grouped_stuff[key]
    if (ARGV[1] != 'e') && (f = group['f'])
      f.sort_by {|i| %w(b l d).index(f[1]) }.each do |i|
        puts i[2]
      end
    end

    if (ARGV[1] != 'f') && group['e']
      puts '----'
      group['e'].each {|e| puts e[1] }
    end

    puts "===="
  end
end

File.open('stuff.txt', 'w') do |file|
  stuff.each {|s| file << s << "\n" }
end