Rails遗留程序中最常犯的错误(上)

近来我一直在对几个遗留项目作维护。

众所周知,处理遗留项目多数时间都感觉糟透了,因为那些代码通常都丑陋不堪而且晦涩难懂。

我决定做一个列表,记录下那些公认的不良实践,或者是我认为不太好的实践,以及如何改良代码来避免这些不良实践。

问题一览

  • 在模型层以外使用查询方法
  • 在视图层使用业务逻辑
  • 使用无意义的方法名和变量名
  • 条件判断时使用unless或者否定的表达式
  • 没有遵循“命令,不要去询问”原则
  • 使用复杂的条件
  • 在模型的实例方法里,本来不需要的时候使用了“self.”
  • 使用条件表达式并且返回了条件
  • 在视图层使用行内样式
  • 在视图层使用JavaScript
  • 调用方法时把另一个方法的调用作为参数
  • 没有使用类来隔离Rake Tasks
  • 没有遵循Sandi Metz的规则

在模型层以外使用查询方法

不好的


; html-script: false ]
# app/controllers/users_controller.rb
class UsersController < ApplicationController

  def index
    @users = User.where(active: true).order(:last_login_at)
  end

end

这段代码不可重用而且难于测试。如果你在别的地方也想查找全部用户并进行排序,就会出现冗余代码。

好的

比起在控制器里使用查询方法,我们的做法是在模型层中使用scope把它们独立出来,就如下例所示。这样做既可以使代码能够复用,又便于代码阅读和测试。


; html-script: false ]
# app/models/user.rb
class User < ActiveRecord::Base

  scope :active, -> { where(active: true) }
  scope :by_last_login_at, -> { order(:lasst_login_at) }

end


; html-script: false ]
# app/controllers/users_controller.rb
class UsersController < ApplicationController

  def index
    @users = User.active.by_last_login_at
  end

end

每当你想用where、order、joins、includes、group、having或者其他查询方法时,记得要把它们放在模型层里。

在视图层使用业务逻辑

不好的


; html-script: false ]
<!-- app/views/posts/show.html.erb -->
...
<h2>
  <%= "#{@comments.count} Comment#{@comments.count == 1 ? '' : 's'}" %>
</h2>
...

初看之下这小段代码似乎没什么问题,但是它会让HTML变得有点难以阅读,而且可以肯定的说一旦你在视图层添加了逻辑代码,那么日后你定会添加更多的逻辑到视图。这段代码还有一个问题,里面的逻辑无法复用,而且不能单独测试。

好的

使用Rails的helper方法把业务逻辑隔离出来


; html-script: false ]
# app/helpers/comments_helper.rb
module CommentsHelper
  def comments_count(comments)
    "#{comments.count} Comment#{comments.count == 1 ? '' : 's'}"
  end
end


; html-script: false ]
<!-- app/views/posts/show.html.erb -->
<h2>
  <%= comments_count(@comments) %>
</h2>

使用无意义的方法名和变量名

不好的


; html-script: false ]
# app/models/topic.rb
class Topic < ActiveRecord::Base

  def self.r_topics(questions)
    rt = []

    questions.each do |q|
      topics = q.topics

      topics.each do |t|
        if t.enabled?
          rt << t
        end
      end
    end

    Topic.where(id: rt)
  end

end

这类遗留代码最主要的问题在于:你需要花费大把时间来搞清楚这些代码的用途。r_topics这个方法是做什么的,rt这个变量又是什么意思。其他的一些变量,比如在代码块里用到的那个,变量名的含义很模糊,这样也使得它们的用途初看起来很难理解。

好的

对方法和变量命名时选那些能表达出其含义的名字。这样更便于其他开发者理解你的代码。


; html-script: false ]
# app/models/topic.rb
class Topic &lt; ActiveRecord::Base

  def self.related(questions)
    related_topics = []

    questions.each do |question|
      topics = question.topics

      topics.each do |topic|
        if topic.enabled?
          related_topics &lt;&lt; topic
        end
      end
    end

    Topic.where(id: related_topics)
  end

end

这样改进的好处在于:

  • 第一次看到方法名时就会对方法返回值有个概念。一个与给定问题集合相关联的主题的集合。
  • 现在你能够了解related_topics表示一个数组,它里面存放了一个与给定问题集合相关联的主题的集合。之前打代码里rt表示什么非常含糊。
  • 使用topic代替之前的t,并用question替换掉q,使得你的代码更便于阅读,因为你不再需要脑补这些变量的用途。现在这些代码已然能够自解释一切。

条件判断时使用unless或者否定的表达式

不好的


; html-script: false ]
# app/services/charge_user.rb
class ChargeUser

  def self.perform(user, amount)
    return false unless user.enabled?

    PaymentGateway.charge(user.account_id, amount)
  end

end

这段代码也许并不难理解,但是使用unless或者否定的条件表达式会稍微增加代码的发复杂度,因为你必须对它要判断的条件自行脑补。

好的

改用if或者肯定的条件表达式之后,上述代码就会好懂得多。


; html-script: false ]
# app/models/user.rb
class User &lt; ActiveRecord::Base

  def disabled?
    !enabled?
  end

end


; html-script: false ]
# app/services/charge_user.rb
class ChargeUser

  def self.perform(user, amount)
    return false if user.disabled?

    PaymentGateway.charge(user.account_id, amount)
  end

end

不觉得这样写代码更易读了吗?我更倾向于使用if而非unless,用肯定的表达式多过肯定的表达式。实在不行就添加个反意的方法,比如我们在User模型里加的那个。

没有遵循“命令,不要去询问”原则

不好的


; html-script: false ]
# app/models/user.rb
class User &lt; ActiveRecord::Base

  def enable!
    update(enabled: true)
  end

end


; html-script: false ]
# app/controllers/users_controller.rb
class UsersController &lt; ApplicationController

  def enable
    user = User.find(params[:id])

    if user.disabled?
      user.enable!
      message = &quot;User enabled&quot;
    else
      message = &quot;User already disabled&quot;
    end

    redirect_to user_path(user), notice: message
  end

end

这里的问题是在不恰当的地方出现了控制逻辑。你先判断了用户是否是不可用,如果的确不可用,就启用这个用户。

好的

比较好的改办法是把控制逻辑放到enable!方法里。


; html-script: false ]
# app/models/user.rb
class User &lt; ActiveRecord::Base

  def enable!
    if disabled?
      update(enabled: true)
    end
  end

end


; html-script: false ]
# app/controllers/users_controller.rb
class UsersController &lt; ApplicationController

  def enable
    user = User.find(params[:id])

    if user.enable!
      message = &quot;User enabled&quot;
    else
      message = &quot;User already disabled&quot;
    end

    redirect_to user_path(user), notice: message
  end

end

现在控制器不用关心user需要满足何种条件才会被启用。相关的判断由User模型和enable!方法来处理。

使用复杂的条件

不好的


; html-script: false ]
# app/controllers/posts_controller.rb
class PostsController &lt; ApplicationController

  def destroy
    post = Post.find(params[:id])

    if post.enabled? &amp;&amp; (user.own_post?(post) || user.admin?)
      post.destroy
      message = &quot;Post destroyed.&quot;
    else
      message = &quot;You&#039;re not allow to destroy this post.&quot;
    end

    redirect_to posts_path, notice: message
  end

end

条件表达式弄的太过复杂了,实际上这里只想知道一件事:用户可以删掉post吗?

好的

从上面的代码我们可以了解到,一个用户需要是post的所有者或者这个用户是管理员,并且post本身也是可用的,才可以删除这个post。最好的做法就是,把这些条件抽取成一个日后可以复用的方法。


; html-script: false ]
# app/models/user.rb
class User &lt; ActiveRecord::Base

  def can_destroy_post?(post)
    post.enabled? &amp;&amp; (own_post?(post) || admin?)
  end

end


; html-script: false ]
# app/controllers/posts_controller.rb
class PostsController &lt; ApplicationController

  def destroy
    post = Post.find(params[:id])

    if user.can_destroy_post?(post)
      post.destroy
      message = &quot;Post destroyed.&quot;
    else
      message = &quot;You&#039;re not allow to destroy this post.&quot;
    end

    redirect_to posts_path, notice: message
  end

end

每当条件表达式里出现了&&或者||,就应该把它们提取为方法,以备以后复用。