新手问题 一小段代码重构

blacktulip · 2013年08月08日 · 最后由 knwang 回复于 2013年08月16日 · 8432 次阅读

需求是这样的:

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 怎么做啊?求助......

共收到 72 条回复

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!"

#1楼 @bhuztez ...... 只能不明觉厉

#5楼 @rainchen 谢谢,这有点高深...

#3楼 @loveky 请问下 self.parse 这个 method 是必须的么?后面直接用 WordsParser.new(words) 的话好像不需要也行。

第一版是最好的...

越写越复杂,这还叫重构么?

要去掉这一堆判断,可以做一个proc到string的map,然后遍历这个map。这种思路在C里用得蛮多,就是用数据结构来替代冗长的算法。

Pad上网,就不贴代码啦~

#12楼 @luikore 一点也不 OO ... 据说也违反了很多 design pattern 之类的

如果回复的类型就这么几种, 感觉不算多, 我倾向于用这个方案:

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 试试我那个吧。

#16楼 @yuan 谢谢,我感觉 parse 不应该连 respond 也管吧。 因为毕竟是 Bob 接受输入然后做出应答,我觉得这个根据不同输入做相应回答的逻辑应该是在 Bob 里面...

#18楼 @blacktulip attr_reader 定义了一个属性 + 它的 getter 方法,所以你不用初始化 instance variable了。

#14楼 如果有说哪里不 OO 的话, 就是那个 class ... design pattern? 难道这个 ruby 是 java 程序员教的... #18楼 @blacktulip 考官大人的心思大概是要你把访问实例变量改成访问 attr_reader... @words => words

#18楼 @blacktulip 让你用 attr_reader 难道要是自封装 @words ?这里倒是不明白了……实例变量在内部直接访问很正常吧。

#22楼 @nightire initialize 省不掉的吧... 要不然 new 的时候没法传 argument 进去啊

#23楼 @luikore 可能是什么 principle 吧,总之意思就是 Bob 管的事情太多了

#25楼 @blacktulip new 只负责初始化对象,用实例方法接收 words,然后返回 response

#27楼 @nightire 那样的话,是不是每次判断都要把 words 传进 parser 实例一次了?

#28楼 @blacktulip 是啊,如果你需要一个 Parser 来负责解析,而不是 Tom 的责任,那么就是如此。

简单地说,Tom 的唯一职责就是把 words 传递给 Parser,然后接收 Parser 返回的结果,你可以把 Parser 想象成为 Tom 的语言中枢处理器。——这就是我之前为啥想给 String 做 MonkeyPatch 的原因,因为我不觉得需要一个单独的 Parser,对这个需求而言。

#29楼 @nightire 谢谢,这个之前被说传来传去传太多了.....

最后这个版本被接受了...

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

#30楼 @blacktulip BTW,15楼是和我说的最接近的方案。试试他那个吧。

蛋疼写法来一个

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

#34楼 @kenshin54 这写法不错,再改改就可以提供 API 了。

#10楼 @blacktulip 恩,这个不是必须的,只是读起来比较舒服,WordParser是个解析器,他应该parse(解析)输入内容,这样比new读起来语义上更通顺吧

c[:when].(words)这个无法接受。难理解。

#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的精髓其实是怎么给抽象的概念起形象的名字...

#37楼 @xds2000 可以写 c[:when].call(words) 或者 c[:when][words] 比较直观

#38楼 在有限的情况下我都这么写的^_^

#5楼 @rainchen 嗯,比较喜欢你的这种写法,扩展性比较好,只关注类型和实现就行!最近也在学习元编程

@rainchen@blacktulip

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

#48楼 @ery 请问下 elsif 为什么少用比较好呢

if elsifif 的阅读成本高一些。

hey 函数中, 不用elsif,也能实现, 所有,只用 if 即可。

阅读复杂度分析 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? 这个板砖是谁丢的?

#53楼 @ery Github 上的人咯...

#12楼 @luikore 👍

第一版最好,这种问题不要over engineering.

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

#58楼 @hisea nice! 结构体的可读性 很强,但是正则表达式的可读性较弱

#58楼 @hisea 这个确实不错,把正则用常量代替可读性就更好了 按楼主的,如果words可以为nil 应该要先to_s一下,然后再匹配下前后空白字符...

实际上写代码,我也直接if-else的写了。。。

在机场,忍不住回一下——

楼主第一版无疑是最直接的。在给定需求不变的情况下,值得改进的地方有两个:

  • 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,最起码也是无功无过。

#61楼 @fsword 我只是回答说有什么办法不pass around word,又没说一定更好。

分离只是为了省掉注释,和你这种方法没有本质区别啊

#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

mark 一下,评论很给力!

交给格式化把.

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

第一版不错,简单明了

需要 登录 后方可回复, 如果你还没有账号请点击这里 注册