Possible help in code refactoring

Question

Sandi Metz says in SOLID OOPS concepts from GORUCO that presence of if..else blocks in Ruby can be considered to be a deviation from Open-Close Principle. What all methods can be used to avoid not-urgent if..else conditions? I tried the following code:

class Fun
   def park(s=String.new)
      puts s
   end
   def park(i=Fixnum.new)
      i=i+2
   end
end

and found out that function overloading does not work in Ruby. What are other methods through which the code can be made to obey OCP?

I could have simply gone for:

class Fun
  def park(i)
      i=i+2 if i.class==1.class 
      puts i if i.class=="asd".class
  end
end

but this is in violation to OCP.

Answer 1

I understand or equal to operation in ruby but can you explain what you have done with:

Object.const_get("#{obj.class}Park").new(obj)

In ruby, something that starts with a capital letter is a constant. Here is a simpler example of how const_get() works:

class Dog
  def bark
    puts 'woof'
  end
end

dog_class = Object.const_get("Dog")
dog_class.new.bark

--output:--
woof

Of course, you can also pass arguments to dog_class.new:

class Dog
  attr_reader :name

  def initialize(name)
    @name = name
  end

  def bark
    puts "#{name} says woof!"
  end
end

dog_class = Object.const_get("Dog")
dog_class.new('Ralph').bark

--output:--
Ralph says woof!

And the following line is just a variation of the above:

Object.const_get("#{obj.class}Park").new(obj)

If obj = 'hello', the first portion:

Object.const_get("#{obj.class}Park")

is equivalent to:

Object.const_get("#{String}Park")

And when the String class object is interpolated into a string, it is simply converted to the string "String", giving you:

Object.const_get("StringPark")

And that line retrieves the StringPark class, giving you:

Object.const_get("StringPark")
            |
            V
      StringPark

Then, adding the second portion of the original line gives you:

      StringPark.new(obj)

And because obj = 'hello', that is equivalent to:

      StringPark.new('hello')

Capice?

Answer 2

Look at the is_a? method

def park(i)
  i.is_a?(Fixnum) ? (i + 2) : i
end

But even better not to check a type, but use duck typing:

def park(i)
  i.respond_to?(:+) ? (i + 2) : i
end

UPD: After reading comments. Yes, both examples above don't solve the OCP problem. That is how I would do it:

class Fun
  # The method doesn't know how to pluck data. But it knows a guy
  # who knows the trick
  def pluck(i)
    return __pluck_string__(i) if i.is_a? String
    __pluck_fixnum__(i) if i.is_a? Fixnum
  end

  private

  # Every method is responsible for plucking data in some special way
  # Only one cause of possible changes for each of them

  def __pluck_string__(i)
    puts i
  end

  def __pluck_fixnum__(i)
    i + 2
  end
end

Answer 3

You could do something like this:

class Parent
  attr_reader :s

  def initialize(s='')
    @s = s
  end

  def park
    puts s
  end
end

class Child1 < Parent
  attr_reader :x

  def initialize(s, x)
    super(s)
    @x = x
  end

  def park
    puts x 
  end
end

class Child2 < Parent
  attr_reader :y

  def initialize(s, y)
    super(s)
    @y = y
  end

  def park
    puts y
  end
end


objects = [
  Parent.new('hello'),
  Child1.new('goodbye', 1),
  Child2.new('adios', 2),
]

objects.each do |obj|
  obj.park
end

--output:--
hello
1
2

Or, maybe I overlooked one of your twists:

class Parent
  attr_reader :x

  def initialize(s='')
    @x = s
  end

  def park
    puts x
  end
end

class Child1 < Parent
  def initialize(x)
    super
  end

  def park
    x + 2 
  end
end

class Child2 < Parent
  def initialize(x)
    super
  end

  def park
    x * 2
  end
end


objects = [
  Parent.new('hello'),
  Child1.new(2),
  Child2.new(100),
]

results = objects.map do |obj|
  obj.park
end

p results

--output:--
hello
[nil, 4, 200]

And another example using blocks, which are like anonymous functions. You can pass in the desired behavior to park() as a function:

class Function
  attr_reader :block

  def initialize(&park)
    @block = park 
  end

  def park
    raise "Not implemented"
  end
end


class StringFunction < Function
  def initialize(&park)
    super
  end

  def park
    block.call
  end
end

class AdditionFunction < Function
  def initialize(&park)
    super
  end

  def park
    block.call 1
  end
end

class DogFunction < Function
  class Dog
    def bark
      puts 'woof, woof'
    end
  end

  def initialize(&park)
    super
  end

  def park
    block.call Dog.new
  end
end


objects = [
  StringFunction.new {puts 'hello'},
  AdditionFunction.new {|i| i+2},
  DogFunction.new {|dog| dog.bark},
]

results = objects.map do |obj|
  obj.park
end

p results

--output:--
hello
woof, woof
[nil, 3, nil]

Answer 4

You could just create handled classes for Fun like so

class Fun
   def park(obj)
    @parker ||= Object.const_get("#{obj.class}Park").new(obj)
    @parker.park 
    rescue NameError => e
        raise ArgumentError, "expected String or Fixnum but recieved #{obj.class.name}"
   end
end

class Park
    def initialize(p)
        @park = p
    end
    def park
        @park
    end
end

class FixnumPark < Park
    def park
        @park += 2
    end
end

class StringPark < Park
end

Then things like this will work

f = Fun.new
f.park("string")
#=> "string"
f.instance_variable_get("@parker")
#=> #<StringPark:0x1e04b48 @park="string">
f = Fun.new
f.park(2)
#=> 4
f.instance_variable_get("@parker")
#=> #<FixnumPark:0x1e04b48 @park=4>
f.park(22)
#=> 6 because the instance is already loaded and 4 + 2 = 6
Fun.new.park(12.3)
#=> ArgumentError: expected String or Fixnum but received Float

Answer 5

With your current example, and wanting to avoid type detection, I would use Ruby's capability to re-open classes to add functionality you need to Integer and String:

class Integer
  def park
    puts self + 2
  end
end

class String
  def park
    puts self
  end
end

This would work more cleanly when altering your own classes. But maybe it doesn't fit your conceptual model (it depends what Fun represents, and why it can take those two different classes in a single method).

An equivalent but keeping your Fun class might be:

class Fun
  def park_fixnum i
    puts i + 2
  end

  def park_string s
    puts s
  end

  def park param
    send("park_#{param.class.to_s.downcase}", param)
  end
end

As an opinion, I am not sure you will gain much writing Ruby in this way. The principles you are learning may be good ones (I don't know), but applying them forcefully "against the grain" of the language may create less readable code, regardless of whether it meets a well-intentioned design.

So what I would probably do in practice is this:

class Fun
  def park param
    case param
    when Integer
      puts param + 2
    when String
      puts param
    end
  end
end

This does not meet your principles, but is idiomatic Ruby and slightly easier to read and maintain than an if block (where the conditions could be far more complex so take longer for a human to parse).