Rails 请教一个重构方案

flowerwrong · May 31, 2014 · Last by mystery replied at June 01, 2014 · 2414 hits

原始代码,严重需要重构,某人说过,代码重复不过 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 不能作为属性调用。 请问怎么重构最好?

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

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

额滴神啊。。。。

#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 Floor has deleted

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

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

14 Floor has deleted
15 Floor has deleted

#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 呀!

You need to Sign in before reply, if you don't have an account, please Sign up first.