新手问题 一小段代码重构
需求是这样的：
Bob is a lackadaisical teenager. In conversation, his responses are very limited.
Bob answers 'Sure.' if you ask him a question.
He answers 'Whatever.' if you tell him something.
He answers 'Woah, chill out!' if you yell at him (ALL CAPS).
He says 'Fine. Be that way!' if you address him without actually saying anything.
lol 我开始就闭着眼睛写啦
# 第一版
class Bob
def hey(words)
if words.nil? || words.strip.empty?
"Fine. Be that way!"
elsif words.upcase == words
"Woah, chill out!"
elsif words[-1] == "?"
"Sure."
else
"Whatever."
end
end
end
然后被拍了无数板砖...... Iterate 了很多次，现在是这样的额
# 第八版
class WordsParser
def silent?(words)
words.nil? || words.strip.empty?
end
def shout?(words)
words.upcase == words
end
def question?(words)
words.end_with?("?")
end
end
class Bob
def hey(words)
@parser = WordsParser.new
if @parser.silent?(words)
"Fine. Be that way!"
elsif @parser.shout?(words)
"Woah, chill out!"
elsif @parser.question?(words)
"Sure."
else
"Whatever."
end
end
end
Anyway... 现在的板砖是这样的：
It's better not to pass words around so much. Can you think of a way to avoid that?
我 think 啊 think 啊也 think 不出来，要把判断抽出去的话，不 pass around 怎么做啊？求助......
WordsParser.new words
就是抽成一个函数吧，比如用 Erlang 可以这么写。
case type_of(Words) of
ask ->
"Sure.";
tell ->
"Whatever.";
yell ->
"Woah, chill out!";
silent ->
"Fine. Be that way!"
end
class WordsParser
def self.parse(words)
return new(words)
end
def initialize(words)
@words = words
end
def silent?
@words.nil? || @words.strip.empty?
end
def shout?
@words.upcase == @words
end
def question?
@words.end_with?("?")
end
end
class Bob
def hey(words)
@parser = WordsParser.parse(words)
if @parser.silent?
"Fine. Be that way!"
elsif @parser.shout?
"Woah, chill out!"
elsif @parser.question?
"Sure."
else
"Whatever."
end
end
end
伪代码大概可以这样写
Bob.responds_to words
class Bob
def self.responds_to words
parser = WordsParser.new words
parser.response.respond
end
end
class Shout; End
class Silence; End
class Question; End
class WordsParser
ALL_RESPONSE = [Shout, Silence, Question]
def initialize words
@words = words
end
def response
ALL_RESPONSE.detect {|r| r.is_response?(@word) }
end
end
有意思的题目，应该是做应答机器人之类的吧，按道理应该用责任链模式，不过这是 ruby，可以简单点：
class WordsParser
attr_reader :type
attr_reader :words
def initialize(words)
@words = words
@type = 'other'
methods.grep(/(.*)_parser/).select do |parser_method|
if send(parser_method)
@type = parser_method.to_s.split("_parser").first
end
end
end
def silent_parser
words.nil? || words.strip.empty?
end
def shout_parser
words.upcase == words
end
def question_parser
words.end_with?("?")
end
end
class Bob
attr_reader :answer
def initialize(words_type)
@answer = send("#{words_type}_answer")
end
def silent_answer
"Fine. Be that way!"
end
def shout_answer
"Woah, chill out!"
end
def question_answer
"Sure."
end
def other_answer
"Whatever."
end
end
["a question?", "something", "WOW", ""].each do |words|
words = WordsParser.new(words)
bob = Bob.new(words.type)
puts %{words: "#{words.words}", type:"#{words.type}", answer: "#{bob.answer}"}
end
# words: "a question?", type:"question", answer: "Sure."
# words: "something", type:"other", answer: "Whatever."
# words: "WOW", type:"shout", answer: "Woah, chill out!"
# words: "", type:"silent", answer: "Fine. Be that way!"
越写越复杂，这还叫重构么？
要去掉这一堆判断，可以做一个 proc 到 string 的 map，然后遍历这个 map。这种思路在 C 里用得蛮多，就是用数据结构来替代冗长的算法。
Pad 上网，就不贴代码啦~
如果回复的类型就这么几种，感觉不算多，我倾向于用这个方案：
class Person < Struct.new(:answer_policy)
def respond(words)
answer_policy.new(words).answer
end
end
class LackadaisicalTeenagerAnswerPolicy < Struct.new(:words)
def answer
case
when nothing?
'Fine, Be that way!'
when yell?
'Woah, chill out!'
when question?
'Sure.'
else
'Whatever'
end
end
def nothing?
words.empty?
end
def question?
words.end_with? '?'
end
def yell?
words == words.upcase
end
end
bob = Person.new(LackadaisicalTeenagerAnswerPolicy)
puts bob.respond('meh')
class Bob
def hey(words)
WordsParser.new(words).parse
end
end
class WordsParser
def initialize(words)
@words = words
end
def parse
if silent?
"Fine. Be that way!"
elsif shout?
"Woah, chill out!"
elsif question?
"Sure."
else
"Whatever."
end
end
private
def question?
@words.end_with? '?'
end
def shout?
@words.upcase == @words
end
def silent?
@words.strip.empty?
end
end
第二版代码的问题在于很多 WordsParser 相关的逻辑被扔给 Bob 了。
我在想啊，需求列出的若干情况不都是对 String 做出响应吗？干脆给 String 打个 Monkey Patch 得了，比如增加一个 String#silly_response 方法。管他是 Tom 还是 Jerry 还是阿猫或阿狗呢，统统管用，任何一个对象只要：
# Pseudo Code
class Anything
def respond_to(words)
words.silly_response
end
end
Monky Patch 怎么打，楼上诸位都写了，我就不重复了。就说我为啥不用一个单独的类处理呢？因为 Conversation 在一个应用里 99% 都是用 String 来表示吧？（或许有 1% 的奇葩），在业务逻辑尚未表示出足够的复杂性之前，我干嘛非得专门写个类来处理本应该 String 自己就能处理的事情？
或许你会觉得 #silly_response 返回的本应该是 Tom 的责任，但我不这么看，我觉得它的返回结果是 a kind of conversation 的责任，而 this kind of conversation 我们用 String 来表达，Tom 仅仅是 响应 this kind of conversation 的一个对象罢了。
不靠谱的念头，仅供调戏用。
#18 楼 @blacktulip attr_reader 定义了一个属性 + 它的 getter 方法，所以你不用初始化 instance variable 了。
#14 楼 如果有说哪里不 OO 的话，就是那个 class ... design pattern? 难道这个 ruby 是 java 程序员教的...
#18 楼 @blacktulip 考官大人的心思大概是要你把访问实例变量改成访问 attr_reader... @words => words
#28 楼 @blacktulip 是啊，如果你需要一个 Parser 来负责解析，而不是 Tom 的责任，那么就是如此。
简单地说，Tom 的唯一职责就是把 words 传递给 Parser，然后接收 Parser 返回的结果，你可以把 Parser 想象成为 Tom 的语言中枢处理器。——这就是我之前为啥想给 String 做 MonkeyPatch 的原因，因为我不觉得需要一个单独的 Parser，对这个需求而言。
最后这个版本被接受了...
class WordsParser
attr_reader :words
def initialize(words)
@words = words
end
def silent?
words.to_s.strip.empty?
end
def shout?
words.upcase == words
end
def question?
words.end_with?("?")
end
end
class Bob
attr_reader :parser
def hey(words)
@parser = WordsParser.new(words)
if parser.silent?
"Fine. Be that way!"
elsif parser.shout?
"Woah, chill out!"
elsif parser.question?
"Sure."
else
"Whatever."
end
end
end
蛋疼写法来一个
class Bob
CONDITIONS = [
{ when: -> words { words.nil? || words.strip.empty? }, answer: "Fine. Be that way!" },
{ when: -> words { words.upcase == words }, answer: "Woah, chill out!" },
{ when: -> words { words.nil? || words[-1] == "?" }, answer: "Sure." },
{ when: -> words { true }, answer: "Whatever." }
]
def hey(words)
CONDITIONS.detect { |c| c[:when].(words) }[:answer]
end
end
#10 楼 @blacktulip 恩，这个不是必须的，只是读起来比较舒服，WordParser 是个解析器，他应该parse(解析)输入内容，这样比 new 读起来语义上更通顺吧
#7 楼 @blacktulip Ruby 类似
response = {
:ask: "Sure.",
:tell: "Whatever.",
:yell: "Woah, chill out!",
:silent: "Fine. Be that way!"
}
def hey(words)
response[type_of(words)]
end
#14 楼 @blacktulip 不用 Erlang 怎么 OO？你的代码没啥问题，能改的最多就是把一个函数拆成两个函数。加更多 class 和是不是 OO 一点关系都没有。Design Pattern 的精髓其实是怎么给抽象的概念起形象的名字...
class WordsParser
attr_reader :words
def initialize(words)
@words = words
methods.grep(/(.*)_parser/).select do |parser_method|
res = send(parser_method)
if res
@type = parser_method.to_s.split("_parser").first
instance_variable_get("@#{@type}").call
break
end
end
other_answer if @type == nil
end
def silent_parser
@silent = lambda{"Fine. Be that way!"} #代码有严格前后要求用快对象来延迟执行代码
words.nil? || words.strip.empty? #若代码没有前后关系的话可以
end #换成instnce_eval
def shout_parser
@shout = lambda{ "Woah, chill out!"}
words.upcase == words
end
def question_parser
@question = lambda{ "Sure."}
words.end_with?("?")
end
def other_answer
"Whatever."
end
end
class Bob
def hey(words)
WordsParser.new(words)
end
end
明显第一版最好啊
answers = [
[/\?$/, "Sure."],
[/^[^a-z]+$/, "Woah, chill out!"],
[//, "Fine. Be that way!"],
[/.*/, 'Whatever.']
]
answers.find{|p| words =~ p[0]}[1]
少用 elsif
class Bob
def hey(words)
if words.nil? || words.strip.empty?
return "Fine. Be that way!"
end
if words.upcase == words
return "Woah, chill out!"
end
if words[-1] == "?"
return "Sure."
end
return "Whatever."
end
end
阅读复杂度分析
if end 最低
if else end 中等
if elsif end 偏高
if elsif elsif end 更高
elsif 越多 阅读的复杂度越高。
#49 楼 @blacktulip 请问 It's better not to pass words around so much. Can you think of a way to avoid that? 这个板砖是谁丢的？
class Bob
def hey(words)
is_silent = words.nil? || words.strip.empty?
if is_silent
return "Fine. Be that way!"
end
is_shout = words.upcase == words
if is_shout
return "Woah, chill out!"
end
is_question = words.end_with?("?")
if is_question
return "Sure."
end
return "Whatever."
end
end
楼上的其实可以写成这样
class Bob
def hey(words)
return "Fine. Be that way!" if words.nil? || words.strip.empty?
return "Woah, chill out!" if words.upcase == words
return "Sure" if words[-1] == "?"
return "Whatever."
end
end
如果不想到处用 word。 可以用 case/switch ruby 的 case/switch 支持 regex
class Bob
def hey(words)
case words
when /^$/
"Fine. Be that way!"
when /^[A-Z]+$/
"Woah, chill out!"
when /.*?&/
"Sure"
else
"Whatever."
end
end
在机场，忍不住回一下——
楼主第一版无疑是最直接的。在给定需求不变的情况下，值得改进的地方有两个：
- if -> case 便于理解逻辑结构
- 使用常量，便于理解判定条件 改后是这样： ```ruby class Bob
ChillOut = /^[^a-z]+$/ Question = /\?$/ NoWord = /^$/
def hey(words) case words when ChillOut "Woah, chill out!" when Question "Sure." when NoWord "Fine. Be that way!" else "Whatever." end end end
这个做法有个前提，借助ruby的case when结构特点充分运用正则，所以后续可能难以实现新需求
#1楼 @bhuztez 的说法可能是错误的，因为逻辑判定的部分代码在这个做法中只是转移到了typeof函数中而已，所以唯一的价值只是分离了“判断”和“输出响应”这两部分逻辑而已，而后续的需求变化还是未知，提前做这个分离可能会over design，最起码也是无功无过。
#62 楼 @bhuztez 如果是这样，那么我不太明白 pass around word 是什么意思，如果指的是 words 这个引用，那么所有的判定代码显然都要被传入的，当然也许我是误解了最后一块板砖 :-D
另外你说的也有道理，借助 typeof 里面的模式匹配，我们就可以从这个 pattern 的返回值中理解它的目的，这个和我用常量记录正则表达式的思路是一样的
BTW：其实用正则在这里确实不是很好，最大的麻烦就是有时候会把简单的条件写复杂（刚才添了几个测试用例，马上就失败了），看到 #45 楼 @quakewang 的做法觉得还不错，也能避免阅读困难，稍微改动如下：
class Bob
Rules = [
[lambda{|words| words.nil? || words.strip.empty?}, 'Fine. Be that way!'],
[lambda{|words| words.upcase == words }, 'Woah, chill out!' ],
[lambda{|words| words[-1] == "?" }, 'Sure.' ],
[lambda{|words| true }, 'Whatever.' ]
]
def hey(words)
Rules.each{|checker, resp| return resp if checker.call(words)}
end
end
lambda 改成新表达方式可能会更简短些
class Bob
Rules = [
[->w{w.nil? || w.strip.empty?}, 'Fine. Be that way!'],
[->w{w.upcase == w }, 'Woah, chill out!' ],
[->w{w[-1] == "?" }, 'Sure.' ],
[->w{true }, 'Whatever.' ]
]
def hey(words)
Rules.each{|checker, answer| return answer if checker.call(words)}
end
end
后续如果有需求变化，只要不是太复杂，可以把 Rules 本身的维护和 Bob 类分离，最后的方向还是职责链 :-P
写了个 DSL 类似的，不过 return 还是比较啰嗦
class WordsParser
def silent?
(@words.nil? || @words.strip.empty?) && @status = :slient
end
def shout?
!@words.strip.empty? && @words.upcase == @words && @status = :shout
end
def question?
@words.end_with?("?") && @status = :question
end
def else?
@status.nil?
end
def with(words, &blk)
@words = words
@status = nil
if block_given?
instance_eval(&blk)
end
end
end
class Bob
def hey(words)
@parser = WordsParser.new
@parser.with(words) do
return "Fine. Be that way!" if silent?
return "Woah, chill out!" if shout?
return "Sure." if question?
return "Whatever." if else?
end
end
end
交给格式化把。
class Bob
def hey(words)
@parser = WordsParser.new
if @parser.silent?(words) then "Fine. Be that way!"
elsif @parser.shout?(words) then "Woah, chill out!"
elsif @parser.question?(words) then "Sure."
else "Whatever."
end
end
end
(ps:本人新手) 我唯一喜欢的就是#63 楼#做法，说下个人意见：这样做的好处是容易对功能进行扩展。举个例子来说：我现在新添加一种，如果我说 ruby python java 等编程语言，要求回复 hello world，那么改怎么写呢。设计原则上有一个叫做 ocp（open－close－priclple），63 楼恰好做到了。
h={}
h[/^$/] = "Fine. Be that way!"
h[/^[A-Z]+$/] = "Woah, chill out!"
h[/.*?&/] = "Sure"
h.default = "Whatever."
print h[words]
关键是 key 和 value 的对应关系。
如果添加一个也很简单：
h[/xxx/] = 'not xxx '
看到 bob.hey(...) 就十分想替换成 bob.answer(...) bob 怎么能 hey 自己呢 洁癖改写如下
class Teenager
def answer(words)
# blah blah blah
# use REGEX indeed.
# blah blah blah
end
end
bob = Teenager.new
bob.answer("How are you?")
类似的
class Baby
def answer(words)
puts "dadada"
end
end
bob = Baby.new
bob.answer("How are you?")
class Bob
def hey(words)
parser words do
silent? "Fine. Be that way!"
shout? "Woah, chill out!"
question? "Sure."
question? "Whatever."
end
end
end
