重构 controller 里写的逻辑代码应该写在哪里？
整个登录的大致代码 讨论下该如何重构吧~麻烦大家了 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 也想不出怎么做才能改进 有人指导下吗~？
#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
#8 楼 @ChanceDoor warden 是 devise 依赖的组件，我不熟悉，可能你要看看 devise 的文档一个规范的 devise 授权模块怎么写。
数据逻辑放 model，业务先 controller 里面提取一个 private 方法；多了放 lib 里面 module；多了提到 gem；多了提到独立系统；仅供参考。
我的做法，如果不超出一个屏幕，可以不动。太长的话，可以考虑提取 private 方法，model 一般是多个控制器要用。module 是提取可重用的逻辑，不常用。
#14 楼 @ChanceDoor 有一种减少流程分支层次的方法是错误先行，此后就可以省去正确的分支；当然这不不一定是一种很好的重构策略，只能算是一种编码风格; 如果重构的话就应该试图去除这些分支，可能就要引入不少设计模式上的东西了。
比如登陆界面需要先调用一个动态令牌的API，
如果动态令牌不通过
动态令牌错误
调用devise的用户名密码验证，
验证不通过
用户名密码错误
判断账号是否存在过期时间，
如果过期时间为nil，
就写入当前时间并成功登陆，
判断是否过期
过期
过期错误
成功登陆
动态令牌这段代码移到 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...
我也是新手，上完 @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
#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/
业务流程化，技术模块化。不管是是业务逻辑还是技术逻辑，尽量作成 DSL，这样技术的人看到的技术，业务的人看到的是业务。
上面这些可以考虑和 devise 一样，作成一个模块插件。