重构 Code smell in Ruby-China code base

fredwu · 2011年11月24日 · 最后由 xdite 回复于 2012年02月04日 · 6927 次阅读

作为一个开源的项目,尤其是用来推广 ruby 的项目,我觉得,应当要特别注意代码的质量。

我刚 fork 了 ruby-china 的 repo,第一眼就是看到了不少的 code smell。

单元测试我之前已经提过了。代码本身有不少地方需要改善,比如:

# 检查用户是否看过
# result:
#   0 读过
#   1 未读
#   2 最后是用户的回复
def user_readed?(user_id)
  uids = Rails.cache.read("Topic:user_read:#{self.id}")
  if uids.blank?
    if self.last_reply_user_id == user_id || self.user_id == user_id
      return 2
    else 
      return 1
    end
  end

  if uids.index(user_id)
    return 0
  else
    if self.last_reply_user_id == user_id || self.user_id == user_id
      return 2
    else 
      return 1
    end
  end
end

这段代码里有不少问题:

1) 'readed' 不是一个英文单词,这样写看上去很怪 2) 0, 1, 2——这个比使用 a, b, c 做 variable 名称还糟糕。请使用 symbol。 3) 多层的 if 嵌套 4) 多个 return

我会慢慢 refactor 一些代码然后提交 pull request。希望大家也都可以注意一下代码的质量啦!抛砖引玉,多多包涵~~ :P

汗!昨天 @xdite 也这么说我代码很多地方乱。 其实这个是以前的老代码,一直没去重构,大家多多海涵

多层 if 的嵌套大家通常是怎么 refactor 的?

我的做法,拆解成多个 if,以上面代码为例

def user_readed?(user_id)
  uids = Rails.cache.read("Topic:user_read:#{self.id}")

  if uids.blank?
    if self.last_reply_user_id == user_id || self.user_id == user_id
      return 2
    else 
      return 1
    end
  end

  if uids.index(user_id)
    return 0
  end

  if self.last_reply_user_id == user_id || self.user_id == user_id
    return 2
  end
  1
end

多个 return 确实不是很好,这个代码里面的一些 if 判断可以使用三元操作符来替代

再改进点应该,不过这个或许太过头了:

def user_readed?(user_id)
  uids = Rails.cache.read("Topic:user_read:#{self.id}")
  return [self.last_reply_user_id,self.user_id].include?(user_id) == true ? 2 : 1 if uids.blank?
  return 0 if uids.include?(user_id)
  return 2 if [self.last_reply_user_id,self.user_id].include?(user_id)
  1
end
def user_readed?(user_id)
  uids = Rails.cache.read("Topic:user_read:#{self.id}")
  result = 1

  if uids.blank?
    result = [self.last_reply_user_id,self.user_id].include?(user_id) ? 2 : 1
  elsif uids.index(user_id)
    result = 0
  end

  result = 2 if [self.last_reply_user_id,self.user_id].include?(user_id)
  result
end

我正在加测试,一会儿我会重构一下然后提交 pull request。:)

#7 楼 @fredwu 这段代码的逻辑要搞清楚哦

是否读过,与是否最后一个回复者,为什么会在一个函数里判断呢?

最后一个回复者,肯定是已读者吧。

def user_read?(user_id)
  uids = Rails.cache.read("Topic:user_read:#{self.id}")
  if self.last_reply_user_id == user_id
    return true
  elsif uids.blank?
    return false
  elsif uids.index(user_id)
    return true
  else
    return false
  end
end

def is_last_reply?(user_id)
  self.last_reply_user_id == user_id
end

不知道这样的代码是不是合适一些?

#9 楼 @zhuangbiaowei 从逻辑上来说这样确实更好。

也许这样是更好的:

def user_read?(user_id)
  uids = Rails.cache.read("Topic:user_read:#{self.id}")
  result = false
  result = true if !uids.blank? and uids.include?(user_id)
end

def is_last_reply?(user_id)
  [self.last_reply_user_id, self.user_id].include?(user_id)
end

先对代码其他地方做了些微调整: https://github.com/huacnlee/ruby-china/pull/35/files

出去跑一圈步回来后继续重构……

基于 Topic id 来 cache 用户的阅读记录在高并发下面会出现写竞争冲突,改成基于 User id 来 cache 用户阅读过帖子的最后一个 reply id,不仅可以解决高并发问题,还能让代码实现更加简单。

#13 楼 @quakewang 赞同,在目前的场景下基于 User 来 cache 确实更好,不仅更符合自然思维且可以解决高并发问题。但如果仅仅使用 User 来 cache 当日后有 “根据 Topic 来寻找所有读过的人” 这样的需求时就无能为力了。

#13 楼 @quakewang 赞,不过这样还有那个绿色(2 最后是用户的回复)的状态无法实现

class User
  def read_topic?(last_reply_id)
    reply_ids = Rails.cache.read("user:#{self.id}:topic_read")
    reply_ids.include?(last_reply_id)
  end

  def read_topic(last_reply_id)
    reply_ids = Rails.cache.read("user:#{self.id}:topic_read")
    reply_ids = [] if reply_ids.blank?
    reply_ids.dup
    reply_ids << last_reply_id
    Rails.cache.write("user:#{self.id}:topic_read",reply_ids)
  end
end

还有一个问题是 ?结尾的 ruby 方法应该是返回 true 或者 false 但 0,1,2 都是 true 啊,没有 follow ruby 的习惯

好题目,ruby 语言比较爽,但是 coder 往往图快不太注意细节,现在是注意这个问题的时候了,也许我们也要多搞一搞 code review

Topic#user_readed? 初步重构完毕:https://github.com/fredwu/ruby-china/commit/5f02b50749f6df945e1478f2106e78eeb68ce51d

重构需要一步一步慢慢来。

以前我一直觉得我有代码洁癖,看了 @fredwu@xdite 的几个提交以后才发现,还有更厉害的,哈哈哈

另外提个小建议:用户的 ID 集最好写'user_ids',因为'uids'一般是用来表示 unique ids 的。:)

看了 @fredwu 的提交后被启发了,有了这个更精简的:

def user_read?(user_id)
  user_id.in?(Rails.cache.read("Topic:user_read:#{self.id}") || [])
end

def is_last_reply?(user_id)
  user_id.in?([self.last_reply_user_id, self.user_id])
end

#15 楼 @huacnlee 用户是否读过某个帖子 和 用户是不是最后一个回帖人 应该拆成 2 个功能点,而且我不明白用户如果是最后一个回帖人,为什么会需要特别显示?

#22 楼 @quakewang ^^ 我都忘了当初设定的原因了,看起来确实用不上

#21 楼 @nowazhu Rails.cache.read,如果可以在 nil 的时候返回 [],就好了。

顺便一提,? 结尾的方法返回的永远该是 boolean 并且对实体本身无影响,可以这么理解吧?

#24 楼 @zhuangbiaowei 返回 [] 逻辑上完全说不通吧⋯⋯ 又不是 xxx.find_by_name 神马的

@fredwu 现在是在国外吧? @xdite 在台湾 这个项目也是俩岸三地的同学们都参与了

#27 楼 @dave 是的,呵呵。

#25 楼 @dotnil method? 返回 Boolean 不动原始数据 method! 直接修改原数据

另外,其实刚开始开发这个社区系统就用上那么多 cache,本身也是一个 smell (premature optimisation). :P 咳咳,我比较洁癖一点,哈,见谅见谅~~

#30 楼 @fredwu 这个地方除了用 cache 还能用什么方法(V2EX 用的是 :hover 设置样式,这个的缺点是换浏览器或是换电脑以后就没有效果了)

#31 楼 @huacnlee @fredwu 的意思应该是项目刚刚开始就做这么多 cache,应该算是提前优化。这其实是没有必要的。

#32 楼 @nowazhu 现在这些 cache 都是需要的,论坛首页已经慢了

#30 楼 @fredwu 我赞同 可以把 Rails.cache.read("Topic:user_read:#{self.id}") 抽象成 api,如 recent_visited_user_ids,user_read?不用关心是否有 cache,以后也能做到不修改 api 来实现 cache

#33 楼 @huacnlee 现在的数据是社区会员 256 人,帖子 155 个,回帖 1129 条。这样就慢了那不是完蛋拉

#31 楼 @huacnlee V2EX 发帖量大,所以换浏览器,链接的 visited 状态不同问题不大,因为基本都有新帖,而且样色不要太亮。现在 ruby-china 发帖量也很大了,可以考虑这个方案。

#36 楼 @Rei 其实这个 cache 还是负担得起的,你不觉得回家看到的阅读状态和公司一样,还有当你有 iPad 访问的时候,是一件很爽的事情么

#37 楼 @huacnlee 确实,我也烦恼 codecampo 在公司和回家看的不一样

#21 楼 @nowazhu 刚刚想了想,现在已经可以简化成两行来实现这项功能。 @fredwu

我覺得這裡面一些設定是去撈 db(MongoDB) 的,本身就會比較慢。作 cache 不是正解,而是在設計時就要找出容易 slow 的地方做成類似 constant 的方式才會快

新的机制: https://github.com/huacnlee/ruby-china/blob/bed5d21cf82d51e01c36778abece8c2af9665a39/app/models/user.rb

原理: 用 user_id + topic_id 作为 key,长期没有被访问的冷数据将会被 Memcached 自动挤掉,仅留下访问比较频繁的那些,此外存放只有一个 last_reply_id 有更新自动就过期了,无需手动清除

我不知道是不是 MongoDB 設置可能本身沒有被優化的關係 (?) 之前下載 zheye.org 這個 project 也是巨慢。

慢到最後我懶得找原因,把所有 db backend 都換成 mysql ....快速無比 T_T

#42 楼 @xdite 这个还是碍于对于 MongoDB 的一些机制不够了解,还有目前 Mongoid 的某些 Bug 也可能是带来性能问题的原因,比如 N+1 的问题

我觉得可以开一个重构专题

#43 楼 @huacnlee n+1 可以打开 identity map 缓解

把 application.html.erb 整塊內容砍掉還是超過 300 ms。感覺 initial 就有東西慢了,不是裡面 code 的問題.....

Rendered home/index.html.erb within layouts/application (15.2ms) Completed 200 OK in 365ms (Views: 19.9ms | Mongo: 0.0ms)

#43 楼 @huacnlee 哦,看了 mention_user_logins 的 cache,cache 更好。其实可以把整个评论片段 cache。

#47 楼 @xdite 这是开发环境还是生产环境?看了下日志没有相差这么悬殊阿

def user_readed?(user_id)
    uids = Rails.cache.read("Topic:user_read:#{self.id}")

    foo = self.last_reply_user_id == user_id || self.user_id == user_id
    return foo ? 2 : 1 if uid.blank? || !uids.index(user_id)
    return 0
end

開發環境,我刻意關掉 memecached 。就看得到速度低落了。

而且其實我還是相當吃驚的。因為我這台是 iMac 8 核 i5 8G,全新安裝.... 蠻確定不是我環境的問題。因為公司的 project 一樣都還是很快...

我剛剛看了很多代碼,測了一下,發現其實很多地方都是 LOGIC IN VIEW,這也可能是緩慢的元兇,因為 View 都是 eval 出來的....

我會慢慢把這一些抽的比較好維護,順便看能不能加速....

可以用 Newrelic 帮助查找程序的性能瓶颈。

#11 楼 @nowazhu result = false result = true if !uids.blank? and uids.include?(user_id)

=>

uids.try(:include?, user_id)

#54 楼 @ashchan try 确实是更好的,不过只能在 rails 中使用,所以没有这样写,昨天我还跟 @huacnlee 说可以用 try 来着。

#54 楼 @ashchan 以后可以多试试这个用法

#9 楼 @zhuangbiaowei 就代码风格而言,这个和我的喜好一样,不喜欢用? : 尤其是套用深的。

只適合 1 層。第二層就是 bad smell 了

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