Rails 开发中经常犯的错误

1 - 在 Model 以外的地方做数据库查询

不好的例子

1
2
3
4
5
6
7
# app/controllers/users_controller.rb
class UsersController < ApplicationController
  def index
    @users = User.where(active: true).order(:last_login_at)
  end

end

上面这段代码复用率不高并且不易测试。如果你在其他地方也想做同样的查询,将会产生重复代码。

比较好的例子

上面的查询我们可以将 controller 里面的查询 通过 model 的 scope 来代替。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
# app/models/user.rb
class User < ActiveRecord::Base

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

end

# 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 等查询的话,记得放 model 里面去。

2 - 在 View 里有业务逻辑处理

不好的例子

1
2
3
4
5
6
7
8
<!-- app/views/posts/show.html.erb -->
...

<h2>
  <%= "#{@comments.count} Comment#{@comments.count == 1 ? '' : 's'}" %>
</h2>

...

上面这段代码看起来没有什么错误,但是看上去 Ruby 代码使得 HTML 可读性不是很好,你应该 停止做这样的事情,这个同样会使得代码复用性不够高,而且不方便独立测试。

比较好的例子

将上面的方法独立到 helper 里面去。

1
2
3
4
5
6
7
8
9
10
11
# app/helpers/comments_helper.rb
module CommentsHelper
  def comments_count(comments)
    "#{comments.count} Comment#{comments.count == 1 ? '' : 's'}"
  end
end

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

改完之后会发现 html 变得简单,可读性变好,helper 里面的代码可以被其他地方使用了, 测试也更加方便了。

另外解决这个问题的方法是使用 decorators,具体使用可以参考 draper 这个 Gem。

3 - 使用无意义的变量名或方法名

不好的例子

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
# 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 方法名什么意思? tr 变量干嘛的? 从第一眼里面无法 猜测到作者的企图。

比较好的例子

我们通过使用有意义的名字定义变量和方法,这个将会帮助开发者非常容易理解你的代码。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
# app/models/topic.rb
class Topic < ActiveRecord::Base

  def self.related(questions)
    related_topics = []

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

      topics.each do |topic|
        if topic.enabled?
          related_topics << topic
        end
      end
    end

    Topic.where(id: related_topics)
  end

end

这样的好处是:

  • 第一次看到这个方法名,就能推测出这个方法会返回什么。 一些关于某个 question 的 topic 集合。
  • related_topics 是存储 question 相关的 topic 的一个数组, 而上面方法里面的 rt 根本就不知道什么意思。
  • 使用 topic 代替变量 t, question 代替 q。会让第一次读你代码的人 非常容易理解,这样就可以让代码来描述自己,这才是最好的。

4 - 使用 Unless 或 否定表达式 在判断条件里

不好的例子

1
2
3
4
5
6
7
8
9
10
# 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 的话代码会更加容易理解了。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
# app/models/user.rb
class User < ActiveRecord::Base

  def disabled?
    !enabled?
  end

end

# 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 或 肯定表达式,如果没有肯定表达式, 可以增加一个,像上面 User Model 改的那样。

5 - controller 有很多业务逻辑判断

不好的例子

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
# app/models/user.rb
class User < ActiveRecord::Base

  def enable!
    update(enabled: true)
  end

end

# app/controllers/users_controller.rb
class UsersController < ApplicationController

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

    if user.disabled?
      user.enable!
      message = "User enabled"
    else
      message = "User already disabled"
    end

    redirect_to user_path(user), notice: message
  end

end

这段问题是你在更新 enabled 之前会查询是不是 disabled。这个判断不应该放在 controller 里面和其他代码在一起。

比较好的例子

应该将这个判断逻辑移动到 model 里面去。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
# app/models/user.rb
class User < ActiveRecord::Base

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

end

# app/controllers/users_controller.rb
class UsersController < ApplicationController

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

    if user.enable!
      message = "User enabled"
    else
      message = "User already disabled"
    end

    redirect_to user_path(user), notice: message
  end

end

这样 controller 代码将更加简单。

6 - 使用复杂的判断条件

不好的例子

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
# app/controllers/posts_controller.rb
class PostsController < ApplicationController

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

    if post.enabled? && (user.own_post?(post) || user.admin?)
      post.destroy
      message = "Post destroyed."
    else
      message = "You're not allow to destroy this post."
    end

    redirect_to posts_path, notice: message
  end

end

上面的判断问了太多的条件了,其实总结起来就是判断这个用户是否可以删除。

比较好的例子

这个判断条件如果我们想以后复用方便的话,最好是移动到 model 里面去。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
# app/models/user.rb
class User < ActiveRecord::Base

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

end

# app/controllers/posts_controller.rb
class PostsController < ApplicationController

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

    if user.can_destroy_post?(post)
      post.destroy
      message = "Post destroyed."
    else
      message = "You're not allow to destroy this post."
    end

    redirect_to posts_path, notice: message
  end

end

所以当你什么时候遇到有 && 或 || 的条件判断的时候,你可以考虑把这个条件抽象成一个方法, 为之后复用使用。

7 - 在 Model 的实例方法里面没有必要使用 self

不太好的例子

1
2
3
4
5
6
7
8
# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    "#{self.first_name} #{self.last_name}"
  end

end

上面的方法比较简单,但没有必要使用 self ,这样会使代码看上去更加简单易读。

比较好的例子

在实例方法里面是没有必要使用 self 的,除非是在赋值的时候必须加上 self。

1
2
3
4
5
6
7
8
# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    "#{first_name} #{last_name}"
  end

end

8 - 使用条件判断并且返回

不太好的例子

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    if name
      name
    else
      "No name"
    end
  end

end

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

  def full_name
    name ? name : "No name"
  end

end

这段代码的问题是增加了控制逻辑,其实是没有必要的。

比较好的例子

下面这种很简单的方式就可以搞定了。

1
2
3
4
5
6
7
8
# app/models/user.rb
class User < ActiveRecord::Base

  def full_name
    name || "No name"
  end

end

|| 和 && 操作符是功能非常强大的,他们可以帮助你简化很多代码。

评论