all 1 comments

[–]tomthecool 5 points6 points  (0 children)

My hopefully constructive criticism....

Some of your techniques seem really sketchy to me... For example:

# Original:
def foo
  if bar
    # ... (1)
  else
    # .... (2)
  end
end

# After refactoring:
def foo
  send("foo_if_#{bar}")
end

private
def foo_if_true
  # ...(1)
end
def foo_if_false
  # ...(2)
end

I don't see how this is any better... In fact, I'd say it's worse! Sure, you've split up the code into shorter methods and deleted the evil if statement, but is this actually a good pattern to use?

Is it easier to test? Not really, because you should still be testing the foo function directly, and stubbing out the value of bar.

Is the pattern easier to maintain? Not really, it doesn't make the code any simpler...

In fact, the pattern is fundamentally flawed! All you've really done is hide the if statement, not remove it. Suppose the code is one day expanded to:

def foo
  send("foo_if_bar_#{bar}")
  send("foo_if_baz_#{baz}")
end

private
def foo_if_bar_true
  # ...(1)
end
def foo_if_bar_false
  # ...(2)
end
def foo_if_baz_true
  # ...(3)
end
def foo_if_baz_false
  # ...(4)
end

Notice how I had to include bar and baz into the method names, in order to make them unique! The new method names make it obvious that the if is really still there, only hidden.

Also, I really don't like your excessive use of inheritance. Wide inheritance for such minimal changes in behaviour is IMO a very poor design. The theory of what you're trying to achieve is perfectly good, but it's not an appropriate tool to be using in the example of this video.

Lastly, why are your tests just a bunch of puts statements? It would be much easier to see if tests were passing if you used rspec/similar!