Rails 请教一个重构方案

flowerwrong · 2014年05月31日 · 最后由 mystery 回复于 2014年06月01日 · 1 次阅读

原始代码,严重需要重构,某人说过,代码重复不过3(说明:各个表和user这个表都是多对多的关系,CourseUser就是关联表)

UN_GRADUATION_TYPE = "本科生"
GRADUATION_TYPE = "研究生"
  @courseUsers = Array.new
  CourseUser.where("check_status < ?", 1).dup.each do |au|
    if au.course.stu_type == GRADUATION_TYPE
      @courseUsers.append(au)
    end
  end

  @aboardUsers = Array.new
  AboardUser.where("check_status < ?", 1).dup.each do |au|
    if au.aboard.stu_type == GRADUATION_TYPE
      @aboardUsers.append(au)
    end
  end

  @awardUsers = Array.new
  AwardUser.where("check_status < ?", 1).dup.each do |au|
    if au.award.stu_type == GRADUATION_TYPE
      @awardUsers.append(au)
    end
  end

  @punishmentUsers = Array.new
  PunishmentUser.where("check_status < ?", 1).dup.each do |au|
    if au.punishment.stu_type == GRADUATION_TYPE
      @punishmentUsers.append(au)
    end
  end

  @researchUsers = Array.new
  ResearchUser.where("check_status < ?", 1).dup.each do |au|
    if au.research.stu_type == GRADUATION_TYPE
      @researchUsers.append(au)
    end
  end

  @serviceUsers = Array.new
  ServiceUser.where("check_status < ?", 1).dup.each do |au|
    if au.service.stu_type == GRADUATION_TYPE
      @serviceUsers.append(au)
    end
  end

  @patentUsers = Array.new
  PatentUser.where("check_status < ?", 1).dup.each do |au|
    if au.patent.stu_type == GRADUATION_TYPE
      @patentUsers.append(au)
    end
  end

我的最初方案:

private
def get_graduation_type_records_without_relation(class_name, str2)
  @tmpUsers = Array.new
  class_name.where("check_status < ?", 1).dup.each do |au|
    if au.str2.stu_type == GRADUATION_TYPE
      @tmpUsers.append(au)
    end
  end
  @tmpUsers
end

问题就在 if au.str2.stu_type == GRADUATION_TYPE, 这里str2不能作为属性调用。 请问怎么重构最好?

共收到 23 条回复

要重构就先写测试吧。不写也不知道你要干什么,重构了也不能保证没问题。

基本上写好了之后好的代码自然就出来了。

额滴神啊。。。。

#1楼 @billy 上面的七段代码重复太多了,我希望写一个函数,然后直接调用函数解决问题。例如

@patentUsers = get_graduation_type_records_without_relation(PatentUser, patent)

#2楼 @liwei78 老哥,啥问题???

主要不知道你那些patent, research什么的是干什么的 。第一接口不统一不好抽象,第二像这个au.course.stu_type本身就违反了oop原则。

你至少在这些model内部先做点工作吧,把这个stu_type代理出去。比如

class  CourseUser
  def stu_type
    course.stu_type
  end
end

这个只是方法之一。貌似表设计得也不是很合理。

eval 里面用 #{str2}

#5楼 @billy 他们都是和user表多对多关联,patent是专利,research是科研。au.course就是CourseUser表中的belongs_to关系,返回course对象,然后course.stu_type表示course对象的学生类型,分类本科生和研究生

随便写了下,看满足要求吗

def get_graduation_type_records_without_relation(type)
  "#{type}_user".classify.constantize.where("check_status < ?", 1).inject([]) do |arr, au|
    if au.send("#{type}".to_sym).try(:stu_type) == GRADUATION_TYPE
      arr << au
    end
  end
end

本科生和研究生是人的属性,放到User里面会感觉更自然些,为什么要放到course里面。

11楼 已删除

#11楼 @liwei78 php java python javascript css html 都会 拼说读写

#10楼 @billy 算是特殊需求,没有学生这一类型

14楼 已删除
15楼 已删除

#14楼 @liwei78 囧,我的check_status全部都在关联表中(一个7个关联表),所以这种解决方案只是简化了一点。

#9楼 @diudiutang 感觉9楼的code是我需要的,就是抽象粗一个函数,然后调用。但是不是很明白,我先看看,七楼的evel貌似也可以。我试试。

好吧。可以做一个duck typing.

module StudentFinder
  GRADUATION_TYPE =  "研究生"
  def student_condition_table
    raise "Override me"
  end

  def masters
    where("check_status < ? AND #{student_condition_table}.stu_type = ?", 1, GRADUATION_TYPE)
    .include(:#『student_condition_table』”)
  end
end

class CourseUser < ActiveRecord::Base
  extend StudentFinder
  def self.student_condition_table
    "courses"
  end
end

# Usage
masters = CourseUser.masters

这个从 Ruby 上来说肯定 eval 最直接。整个结构都改掉没啥必要。

@blacktulip eval is evil, don't introduce it when you can.

#20楼 @billy 就那么两行能出啥问题

UN_GRADUATION_TYPE = "本科生"
GRADUATION_TYPE = "研究生"

def get_graduation_type_relation_records class_name, fun
  return [] unless class_name = class_name.safe_constantize
   # .clone 可以拷到 Engienclass  而 dup 不可以,避免加其它单例方法失败。
  class_name.where("check_status < ?", 1).clone.inject([]) do |arr, klass|
    arr << klass if GRADUATION_TYPE == klass.send(fun).try(:stu_type)
  end
end
@courseUsers = get_graduation_type_relation_records 'CourseUser', 'course'
@serviceUsers = get_graduation_type_relation_records 'ServiceUser', 'service'
@patentUsers get_graduation_type_relation_records 'PatentUser', 'patent'

#9楼 @diudiutang 感谢大家,特地感谢楼主。我的code是:

private
def get_graduation_type_records(class_name, field_name)
  @tmpUsers = Array.new
  class_name.where("check_status < ?", 1).dup.each do |au|
    if au.send("#{field_name}".to_sym).try(:stu_type) == GRADUATION_TYPE
      @tmpUsers.append(au)
    end
  end
  @tmpUsers
end
#usage
@patentUsers = get_graduation_type_records(PatentUser, "patent")

eval我没做出来,主要是不熟ruby。对于eval is devil,我觉得见仁见智吧。在js中eval也被说成devil,但是我一个月前读百度编辑器(UEditor)的时候,看到源码中就有lots of eval。

每个子类分别 delegate 出去就好了…… 通用方法里只需要用 au.stu_type

#23楼 @mystery LZ的code报错了,

arr << klass if GRADUATION_TYPE == klass.send(fun).try(:stu_type)

error code:

NoMethodError (undefined method `<<' for nil:NilClass)

也就是arr是nil,所以不存在<<方法。

#25楼 @flowerwrong 麻烦楼主代码贴出来看一下 inject([]) do |arr, klass| arr 此时不应该是nil呀!

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