Skip to content

Instantly share code, notes, and snippets.

@IanWhitney
Last active January 9, 2024 12:21
Show Gist options
  • Save IanWhitney/5e2f3ff7099768f3cf40 to your computer and use it in GitHub Desktop.
Save IanWhitney/5e2f3ff7099768f3cf40 to your computer and use it in GitHub Desktop.
class Bob
def reply_to(statement)
public_send("reply_to_#{statement.class}".downcase.to_sym)
rescue NoMethodError
default_reply
end
def reply_to_silence
"Fine. Be that way!"
end
def reply_to_yelling
"Woah, chill out!"
end
def reply_to_question
"Sure."
end
def default_reply
"Whatever."
end
end
class Statements
def self.all
[Question, Yelling, Silence, NullStatement]
end
end
Statement = Struct.new(:statement)
class Question < Statement
def self.match?(statement)
statement.end_with?("?")
end
end
class Yelling < Statement
def self.match?(statement)
statement.upcase == statement && statement.downcase != statement
end
end
class Silence < Statement
def self.match?(statement)
statement.strip.empty?
end
end
class NullStatement < Statement
def self.match?(statement)
true
end
end
class StatementFactory
def self.build(statement)
Statements.all.detect { |s| s.match?(statement) }.new(statement)
end
end
@rmm5t
Copy link

rmm5t commented Mar 12, 2015

I tweeted about this.

Conditionals are not bad! This is not a better design than https://gist.github.com/rmm5t/cf74958eb839b1403ad4

class Bob
  def respond_to(input)
    @input = input.strip

    return "Fine. Be that way!" if silence?
    return "Whoa, chill out!"   if shouting?
    return "Sure."              if question?
    "Whatever."
  end

  def silence?
    @input.empty?
  end

  def shouting?
    @input == @input.upcase && @input != @input.downcase
  end

  def question?
    @input.end_with?("?")
  end
end

New statement types in the "refactored" non-conditional version requires change in 3 places! (Though, that could be refactored to just 2). Standard case-statement or if-statements still only require changes in 2 places for a new statement type.

...and all the logic and changes are encapsulated in one class as opposed to N+1 classes.

Which is more readable? Which is more maintainable? I argue simpler is much better here.

Granted, Bob is a contrived example, but preaching that conditionals are bad is damaging to the less experienced programmers out there.

@rmm5t
Copy link

rmm5t commented Mar 12, 2015

Btw, I like the suggested refactor by @mereghost here, because...

  1. There's no need to have a separate class/namespace for the "factory," and I think the Statement.build class method is much more discoverable than StatementFactory.build. I'd also argue it's more idiomatic ruby too.
  2. It reduces the number of changes necessary for new statement types, hence, allowing for easier expansion.

@jpfuentes2
Copy link

Case statements are fine too:

(defn respond-to [input]
  (cond
    (empty? input) "Fine. Be that way!"
    (= input (.toUpperCase input)) "Whoa, chill out!"
    (= \? (last input)) "Sure."
    :else "Whatever."))

@IanWhitney
Copy link
Author

I still don't get why gists don't notify me of comments. Maybe there's a setting I've missed.

Anyway.

Thanks for commenting, and thanks for reading my post.

As with anything the final, absolute, unarguable answer is it depends. If you have a conditional that will never change, then cooli-o, leave it as a conditional. If you have a conditional that is going to change/expand/complexify/etc then maybe the conditional is not your friend. The 'quality' of a design can really only be judged in the context of the kinds of changes your system will have to undergo.

@hehejo
Copy link

hehejo commented Mar 13, 2015

Ok, that's a nice concept. But as usual: What if the statement is “ARE YOU YELLING?”…
Instead of own classes I would add a type to the Statement. Statement = Struct(:type, :statement)

@amcaplan
Copy link

So many comments here...

I'll just say I have 2 major complaints here:

  1. While I do think case statements are evil, and conditionals which lots of conditions are ugly, I don't think this is a very Ruby solution. It rails against Ruby's trusting and duck typing nature. We've basically now forced Bob to know the names of lots of other classes.

  2. I also don't love having the object send itself messages and rescue NoMethodError. Errors shouldn't be used for control flow, and sending messages is cool but there are better ways of doing this.

So first, here's how I would handle it just addressing issue #2:

class Bob
  REPLIES = Hash.new("Whatever.").merge!(
    'silence' => 'Fine. Be that way!',
    'yelling' => 'Woah, chill out!',
    'question' => 'Sure.'
  )

  def reply_to(statement)
    REPLIES[statement.class.downcase]
  end 
end

Simple and concise.

Next, I'd agree with @hehejo that the StatementFactory's job is to know what kind of Statement to build; Statements themselves are just data containers and should be pretty dumb. So I'd recommend something like this:

Statement = Struct.new(:type, :statement)

class StatementFactory
  def self.build_statement(phrase)
    Statement.new(type_for_statement(phrase), phrase)
  end

  def self.type_for_statement(phrase)
    if statement.strip.empty?
      :silence
    elsif statement.upcase == statement && statement.downcase != statement
      :yelling
    elsif statement.end_with?('?')
      :question
    end
  end
end

That looks oddly similar to a previous example... note, though, that we don't need a NullStatement, because ::type_for_statement will return a nil type. Here's how we would use it in a slightly modified/simplified Bob:

class Bob
  REPLIES = Hash.new("Whatever.").merge!(
    silence: 'Fine. Be that way!',
    yelling: 'Woah, chill out!',
    question: 'Sure.'
  )

  def reply_to(statement)
    REPLIES[statement.type]
  end 
end

But we can do better. Here's a somewhat nicer version of StatementFactory:

Statement = Struct.new(:type, :statement)

class StatementFactory
  STATEMENT_TYPES = {
    silence: -> (phrase) { phrase.strip.empty? },
    yelling: -> (phrase) { phrase.upcase == phrase && phrase.downcase != phrase },
    question: -> (phrase) { phrase.end_with?('?') }
  }

  def self.build_statement(phrase)
    Statement.new(type_for_statement(phrase), phrase)
  end

  def self.type_for_statement(phrase)
    STATEMENT_TYPES.each_key.find { |type|
      STATEMENT_TYPES[type].call(phrase)
    }
  end
end

I think an if/elsif construct as above would be fine for StatementFactory, but if we expected to add more statement types, this gives us one place where we can add new types. We could use methods, but since they're all doing basically the same thing, I like having a hash where you can easily group and see the correspondence between the symbol representing a statement type, and a lambda representing the conditions that make it relevant.

This solution does rely on Ruby maintaining the order of hashes, which it does in recent versions.

I prefer this solution because it's easily extensible, but doesn't overwhelm us with tons of classes. Data containers should contain data, not be data.

And it doesn't hurt that it's significantly more concise than the previously proposed solution. 😄

@disbelief
Copy link

@amcaplan I agree with you regarding #2 — I don't love the use of exceptions for flow control. However I think this is a contrived example with the stated goal of zero conditionals, so maybe we shouldn't judge too harshly.

However regarding your gripe in #1:

We've basically now forced Bob to know the names of lots of other classes.

I don't see how Bob having a REPLIES hash with keys matching the names of Statement classes is any different from Bob having methods matching those names.

In addition, I don't love the Factory, nor the need to maintain an array of Statements in a separate class. Now if a programmer creates a new kind of Statement, she also needs to know about putting it into an array in a separate class. @mereghost's solution leaving instantiation to the superclass and using inherited? to keep track of all descendants seems the ideal solution to me.

@rmcsharry
Copy link

@mereghost's answer is pretty interesting but I think it has a typo?

I think it should be like this:

self.inherited(klass)

No question mark in the name.

Ref: https://apidock.com/ruby/Class/inherited

@mereghost
Copy link

@rmcsharry Yep. >.<

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment