2
\$\begingroup\$

I'm working on an article about encapsulation in OOP, and I'm using the FizzBuzz problem to demonstrate what I think is so great about it. I'm hoping to share this with developers that might also be intermediate, as a way to start building a bridge for understanding common OO principles.

To this end, any feedback, criticism, or questions are greatly appreciated.

The concerns I'm trying to extract and encapsulate are:

  • The printing of the numbers
  • The iteration of the collection
  • The logic for determining what should be returned
class FizzNumber

  FIZZ_MAP = {
    "Fizzbuzz" => :fizzbuzz?,
    "Fizz" => :fizz?,
    "Buzz" => :buzz?
  }

  def initialize(number)
    @number = number
  end

  def fizz?
    @number % 3 == 0  
  end

  def buzz?
    @number % 5 == 0
  end

  def fizzbuzz?
    fizz? && buzz?
  end

  def fizz_value
    value = FIZZ_MAP.find { |key, value| send(value) }
    value&.first || @number.to_s
  end
end

class FizzBuzzer
  include Enumerable

  def initialize(collection)
    @collection = collection
  end

  def each
    @collection.each do |item|
      yield FizzNumber.new(item)
    end
  end
end

fizzer = FizzBuzzer.new((1..100).to_a)
puts fizzer.map(&:fizz_value).join("\n")

\$\endgroup\$
0
\$\begingroup\$

To this end, any feedback, criticism, or questions are greatly appreciated.

Here are some ideas to improve the code.

Split printing and logic

Might be debatable but from my POV the

The printing of the numbers

and

The logic for determining what should be returned

are not split.

One of the goals of object oriented design is to make the code easier to change. So imagine we want to implement now to print Boom if a number number % 10 == 0. How would we do this? We would need to implement a BoomNumber class and change FizzBuzzer and might even change the main method because fizz_value does not make sense anymore. Not good!

In this example we can just swap out the builder and it would work.

class PrintableNumber
  def initialize(name)
    @name = name
  end

  def to_s
    @name
  end
end

class BoomNumberBuilder
  def initialize(number)
    @number = number
  end

  def build
    PrintableNumber.new(name)
  end

  private

  attr_reader :number

  def name
    if boom?
      "Boom"
    else
      number
    end
  end
  
  def boom?
    number % 10 == 0
  end
end

class FizzNumberBuilder
  def initialize(number)
    @number = number
  end

  def build
    PrintableNumber.new(name)
  end

  private

  attr_reader :number

  def name
    if fizzbuzz?
      "FizzBuzz"
    elsif fizz?
      "Fizz"
    elsif buzz?
      "Buzz"
    else
      number
    end
  end

  def fizzbuzz?
    fizz? && buzz?
  end

  def fizz?
    number % 3 == 0
  end

  def buzz?
    number % 5 == 0
  end
end

class PrintableNumberCollector
  include Enumerable

  def initialize(collection, builder = FizzNumberBuilder)
    @collection = collection
    @builder = builder
  end

  def each
    collection.each do |item|
      yield builder.new(item).build
    end
  end

  private

  attr_reader :collection, :builder
end

puts "== Fizzer =="
fizzer = PrintableNumberCollector.new((1..10).to_a)
puts fizzer.map(&:to_s).join("\n")

puts "== Boomer =="
boomer = PrintableNumberCollector.new((1..10).to_a, BoomNumberBuilder)
puts boomer.map(&:to_s).join("\n")

 Implement common interface

In your FizzNumber class you implement a fizz_value method which you then use in your main fizzer.map(&:fizz_value).join("\n"). This creates an unnecessary coupling. If you look into the method, you can already see what the method name should be number.to_s

  def fizz_value
    value = FIZZ_MAP.find { |key, value| send(value) }
    value&.first || @number.to_s
  end
end

so a better function name would be

def to_s
  value&.first || @number.to_s
end

private

def value
  FIZZ_MAP.find { |key, value| send(value) }
end

Avoid meta programming

Try to avoid meta programming (send) if not absolutely necessary. It makes the code a lot harder to read. You only have 4 different cases which can easily reflected with an if else.

  def to_s
    if fizzbuzz?
      "FizzBuzz"
    elsif fizz?
      "Fizz"
    elsif buzz?
      "Buzz"
    else
      number
    end
  end

Use getter and setter

Instead of using instance variables @collection you should try to use getter / setter methods. If you need to do e.g. validations or some processing of the variable, you only need to change one place.

class FizzBuzzer
  attr_reader :collection

  def each
    collection.each do |item|
      yield FizzNumber.new(item)
    end
  end
end

Use simple delegator

Instead of implementing the each method you could also use the SimpleDelegator module.

https://ruby-doc.org/stdlib-2.5.1/libdoc/delegate/rdoc/SimpleDelegator.html

\$\endgroup\$
1
  • \$\begingroup\$ Hey Thanks! When I said separate the printing from the logic, I just meant that the consumer/executing context should be responsible for actually printing the numbers. It appears you do that in your example as well. I'll definitely look through this. Good stuff. \$\endgroup\$ Jun 9 '20 at 15:22

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.