重构 controller 里写的逻辑代码应该写在哪里?

chancedoor · 2013年04月23日 · 最后由 mvj3 回复于 2013年06月28日 · 7555 次阅读

整个登录的大致代码 讨论下该如何重构吧~麻烦大家了 12 楼有补充 经过大家的提点 第一轮重构如下 第二轮重构如下 将 deadline 的业务逻辑放进 model 并使用 before_filter 进行令牌验证

#app/controller/sessions_controller.rb
class SessionsController < Devise::SessionsController
  before_filter :check_ikey,:only => [:create]

  def create
    self.resource = warden.authenticate!(auth_options)
    sign_in(resource_name,resource)
    if(resource.check_deadline)
      set_flash_message(:notice, :signed_in) if is_navigational_format?
      respond_with resource, :location => after_sign_in_path_for(resource)
    else
      sign_out
      redirect_to login_path,:alert =>"This account has expired."
    end
  end

  private
  def check_ikey
    param={
        'AuthName' =>params[:user][:email],
        'AuthPasswd' => params[:user][:ikey],}
    client=Savon.client(wsdl: "#{request.protocol}#{request.host_with_port}/Auth.wsdl")
    result=client.call(:auth,:message=>param)
    if(result.body[:auth_res].to_i <0)
      redirect_to login_path, alert: "Ikey error : Invalid username or ikey."
    end
  end

end
#app/models/user.rb
class User < ActiveRecord::Base
   attr_accessible :deadline

   def check_deadline
     if(deadline!=nil)
       deadline>Time.now
     else
       self.update_attributes(:deadline=>Time.now+1.month)
       true
     end
   end

end

代码简洁了太多!感谢大家 接下来想知道这些毕竟还是逻辑代码 是不是应该移出 controller 呢?是的话应该放到哪里去? 好多了! 还有问题就是在过期验证之前其实就已经获得权限了 然后用 sign out 删除 这样总归不太好 但因为用了 devise 也想不出怎么做才能改进 有人指导下吗~?

放到 lib

#1 楼 @Rei 来不及自己试了 myapp/lib/my/login.erb

module login
  def login_vertification
    param={
        'msAuthName' =>params[:user][:email],
        'msAuthPasswd' => params[:user][:password],
    }
    client=Savon.client(wsdl: "#{request.protocol}#{request.host_with_port}/ikeyAuth.wsdl")
    result=client.call(:token_auth,:message=>param)
    if
      ...
    else
      respond_with resource, :location => after_sign_in_path_for(resource)
    end
  end
end

是这样吗? session controller 里怎么写? 麻烦啦!

# lib/my/login.rb
module My
  module Login
    def self.login_vertification(email, password)
      # login logic
    end
  end
end
class SessionsController < Devise::SessionsController
  def create
     if My::Login.login_vertification params[:email], params[:password]
       # ...
     else
       # ...
     end
  end
end

#3 楼 @Rei 怎么试都不行啊。。。 undefined method 'login_vertification' for My::Login:Module

#4 楼 @ChanceDoor 是不是漏了个 self

哈 这么明显。。。

#5 楼 @Rei 不是 貌似是我的模块名有重名的。。。我试试换个名称

#6 楼 @tumayun 新手 见谅啊~

#5 楼 @Rei 应该是解决了 就是这句有问题 self.resource = warden.authenticate!(auth_options) 不能放 module 吗?

#8 楼 @ChanceDoor warden 是 devise 依赖的组件,我不熟悉,可能你要看看 devise 的文档一个规范的 devise 授权模块怎么写。

#9 楼 @Rei 谢谢!终于可以把代码从 controller 里拿出来了!TAT 纠结了好久 恩 我还是先把这句放 controller 里吧

数据逻辑放 model,业务先 controller 里面提取一个 private 方法;多了放 lib 里面 module;多了提到 gem;多了提到独立系统;仅供参考。

#11 楼 @as181920 但是我现在的 controller 肥大似乎不好分离数据逻辑,反正感觉很难下手

比如登陆界面需要先调用一个动态令牌的API,
      如果通过
             调用devise的用户名密码验证,
                   验证通过的话
                          判断账号是否存在过期时间,
                          如果过期时间为nil,
                                就写入当前时间并成功登陆,
                          如果过期时间不为nil,
                                判断是否过期
                                过期
                                      过期错误
                                不过期
                                      成功登陆
                    验证不通过
                           用户名密码错误
        如果动态令牌不通过
               动态令牌错误    

总之就是 1.动态令牌验证 2.用户名密码验证 3.账号过期时间验证 应该怎么重构是最佳的呢?

#9 楼 @Rei

我的做法,如果不超出一个屏幕,可以不动。太长的话,可以考虑提取 private 方法,model 一般是多个控制器要用。module 是提取可重用的逻辑,不常用。

#13 楼 @chenge 谢谢~ 有时间的话帮我看看 12 楼的重构实例吧

#14 楼 @ChanceDoor 有一种减少流程分支层次的方法是错误先行,此后就可以省去正确的分支;当然这不不一定是一种很好的重构策略,只能算是一种编码风格; 如果重构的话就应该试图去除这些分支,可能就要引入不少设计模式上的东西了。


比如登陆界面需要先调用一个动态令牌的API,
    如果动态令牌不通过
        动态令牌错误    

    调用devise的用户名密码验证,
        验证不通过
          用户名密码错误

        判断账号是否存在过期时间,
            如果过期时间为nil,
                就写入当前时间并成功登陆,

            判断是否过期
            过期
                 过期错误

            成功登陆

#15 楼 @donnior 我也是这么觉得 分支太深了 可是因为使用了 devise 也不太容易随自己的意来做 可能还是要好好看看 devise 验证的那两句 然后用返回值来做 switch 分支 不过主要还是说像这样的代码 应该放在哪里比较好呢?

过期时间算数据逻辑吧 应该放到 model 里吗?动态令牌 API 虽然一个项目只用一次 但如果考虑到别的项目也可以用的话 似乎应该做个 Gem 或者 Module? 总之现在最主要的是把 controller 解放出来 即使不是最好的方法 只要 controller 精简就行

动态令牌这段代码移到 before_filter 过期时间写成 devise 的扩展模块,这样你就不需要复写 devise 的 sesison controller create 方法了。

class SessionsController < Devise::SessionsController
  before_filter :check_ikey, :only => [:create]

  def check_ikey
  #...
  end
end

#16 楼 @ChanceDoor 真正重构的话就应该试图去除这些分支,可能就要引入不少设计模式上的东西了;就这个问题我可能就会采用链式的简化处理方法,基本原理就是

login_result = 动态令牌检查器 && 验证检查器 && 过期时间检查器

这其中的每一个检查器都可以封装成一个完整的对象并在其中处理逻辑和错误,并返回 bool 之类的标志以便决定下一个检查器;好处是 caller 完全移除 if-else,具体每个检查器的判断逻辑也很简单

我没用过 devise,以上说法也只是根据一般代码的设计思路而言,不一定真正适用你的具体场景。

至于放哪儿,实际上我觉得根据你的场景而言,首先这一段逻辑你会重用么?它是否是属于业务层的逻辑?或者是其他要考虑的?然后再决定是放在 controller,还是 model,是否需要封装成一个 class,或者是要变成 lib,更或者是 module...

#17 楼 @quakewang #18 楼 @donnior 谢谢大家的教导 我先尝试一下

#18 楼 @donnior #17 楼 @quakewang #13 楼 @chenge 根据你们的指导修改了代码 现在至少分支上清晰了

我也是新手,上完 @knwang 老师的课程后学到的,用了 service object 和 domain object 仅供参考(对 devise 和 Savon 不熟悉,而且还没有完全掌握)。

#app/controller/sessions_controller.rb
class SessionsController < Devise::SessionsController
  def create
    #首先调用API验证动态令牌
    param={
        'AuthName' => params[:user][:email],
        'AuthPasswd' => params[:user][:ikey],
    }
    client = Savon.client(wsdl: "#{request.protocol}#{request.host_with_port}/Auth.wsdl")
    self.resource = warden.authenticate!(auth_options)

    result = LoginDeduction.new(client.call(params), resource, sign_in(resource_name, resource)).deduct

    if !result.dynamic_validation_pass?
      redirect_to login_path, alert: "Ikey error : Invalid username or ikey."   #动态令牌验证失败
    elsif result.sign_in? && result.term_expired?
      sign_out
      redirect_to login_path, alert: "Account error : This account is expired."
    else
      set_flash_message(:notice, :signed_in) if is_navigational_format?
      respond_with resource, location: after_sign_in_path_for(resource)
    end
  end
end

#app/serives/login_deduction.rb
class LoginDeduction
  attr_reader :dynamic_validation_result, :user_term, :sign_in_result

  alias_method :sign_in?, :sign_in_result
  alias_method :dynamic_validation_pass?, :dynamic_validation_result

  def initialize(result, resource, sign_in_result)
    @dynamic_validation_result = result
    @user_term = UserTermPolicy.new(resource.deadline)
    @sign_in_result = sign_in_result
  end

  def deduct
    if dynamic_validation_pass?
      deduct_user_sign_in
    else
      LoginResult.new(dynamic_validation: false, sign_in: false)
    end
  end

  def deduct_user_sign_in
    if sign_in?                      #devise的登录成功
      user_term = UserTermPolicy.new(resource.deadline)
      if user_term.has_a_deadtime?            #如果有过期时间
        if user_term.is_expired?                 #如果已过期 登录失败
          LoginResult.new(sign_in: true, dynamic_validation: true, expired: true)
        else
          LoginResult.new(sign_in: true, dynamic_validation: true)
        end
      else
        UserTerm.new(user).update_deadline   # 如果没有过期时间 即新账号 就写入过期时间为一个月后,并登陆成功
        LoginResult.new(sign_in: true, dynamic_validation: true)
      end
    else
      LoginResult.new(sign_in: false, dynamic_validation: true)
    end
  end
end

class LoginResult
  attr_reader :dynamic_validation, :sign_in, :expired

  alias_method :dynamic_validation_pass?, :dynamic_validation
  alias_method :sign_in?, :sign_in
  alias_method :term_expired?, :expired

  def initialize(options={})
    @dynamic_validation = options[:dynamic_validation]
    @sign_in = options[:sign_in]
    @expired = options[:expired]
  end
end

#app/models/user_term.rb
class UserTerm
  attr_accessor :user

  def initialize(user)
    @user = user
  end

  def update_deadline
    user.update_attributes(deadline: Time.now + 1.month)
  end
end
#app/models/user_term_policy.rb
class UserTermPolicy
  attr_reader :deadtime

  def initialize(deadtime)
    @deadtime = deadtime
  end

  def has_a_deadtime?
    deadtime != nil
  end

  def is_expired?
    deadtime <= Time.now
  end
end

@ChanceDoor 没有看到你已经重构一轮了,我这个是从你最初的代码开始重构的。

#22 楼 @poiyzy 好的!谢谢 我看看

#23 楼 @ChanceDoor 刚才有一个 initialize 的错误,重新修改了。

#24 楼 @poiyzy 你写的我看花眼了啊。。。你是直接自己写了个登录验证没有用 devise? 我又重构了

#17 楼 @quakewang 麻烦帮忙看看主帖我现在的代码 已经好多了 也按你说的用 before_filter 来处理 check_ikey 了 还有你说的把 check_deadline 做成 devise 的扩展模块是什么意思?怎么操作?

#25 楼 @ChanceDoor 用到 devise 了呀,这个是课程上讲的一些重构的知识 主要是 Object Oriented 的概念,好处是这样做虽然代码增加了很多,但是如果重用其中的方法会很方便,而且随着 app 不断庞大结构还是会很清晰。

我可能还是没有掌握好这种模式,所以你看眼花了。

这是 @knwang 推荐的扩展阅读:

http://blip.tv/rubynation/jeff-casimir-fat-models-aren-t-enough-5562605 http://blog.codeclimate.com/blog/2012/10/17/7-ways-to-decompose-fat-activerecord-models/

#27 楼 @poiyzy Thanks!看懂些了

业务流程化,技术模块化。不管是是业务逻辑还是技术逻辑,尽量作成 DSL,这样技术的人看到的技术,业务的人看到的是业务。

上面这些可以考虑和 devise 一样,作成一个模块插件。

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