近來我一直在對幾個遺留專案作維護。
眾所周知，處理遺留專案多數時間都感覺糟透了，因為那些程式碼通常都醜陋不堪而且晦澀難懂。
我決定做一個列表，記錄下那些公認的不良實踐，或者是我認為不太好的實踐，以及如何改良程式碼來避免這些不良實踐。
問題一覽
- 在模型層以外使用查詢方法
- 在檢視層使用業務邏輯
- 使用無意義的方法名和變數名
- 條件判斷時使用unless或者否定的表示式
- 沒有遵循“命令，不要去詢問”原則
- 使用複雜的條件
- 在模型的例項方法裡，本來不需要的時候使用了“self.”
- 使用條件表示式並且返回了條件
- 在檢視層使用行內樣式
- 在檢視層使用JavaScript
- 呼叫方法時把另一個方法的呼叫作為引數
- 沒有使用類來隔離Rake Tasks
- 沒有遵循Sandi Metz的規則
在模型層以外使用查詢方法
不好的
|
1 2 3 4 5 6 7 8 9 |
; 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把它們獨立出來，就如下例所示。這樣做既可以使程式碼能夠複用，又便於程式碼閱讀和測試。
|
1 2 3 4 5 6 7 8 |
; 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 |
|
1 2 3 4 5 6 7 8 9 |
; 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或者其他查詢方法時，記得要把它們放在模型層裡。
在檢視層使用業務邏輯
不好的
|
1 2 3 4 5 6 7 |
; html-script: false ] <!-- app/views/posts/show.html.erb --> ... <h2> <%= "#{@comments.count} Comment#{@comments.count == 1 ? '' : 's'}" %> </h2> ... |
初看之下這小段程式碼似乎沒什麼問題，但是它會讓HTML變得有點難以閱讀，而且可以肯定的說一旦你在檢視層新增了邏輯程式碼，那麼日後你定會新增更多的邏輯到檢視。這段程式碼還有一個問題，裡面的邏輯無法複用，而且不能單獨測試。
好的
使用Rails的helper方法把業務邏輯隔離出來
|
1 2 3 4 5 6 7 |
; html-script: false ] # app/helpers/comments_helper.rb module CommentsHelper def comments_count(comments) "#{comments.count} Comment#{comments.count == 1 ? '' : 's'}" end end |
|
1 2 3 4 5 |
; html-script: false ] <!-- app/views/posts/show.html.erb --> <h2> <%= comments_count(@comments) %> </h2> |
使用無意義的方法名和變數名
不好的
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 |
; 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這個變數又是什麼意思。其他的一些變數，比如在程式碼塊裡用到的那個，變數名的含義很模糊，這樣也使得它們的用途初看起來很難理解。
好的
對方法和變數命名時選那些能表達出其含義的名字。這樣更便於其他開發者理解你的程式碼。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 |
; html-script: false ] # 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 |
這樣改進的好處在於：
- 第一次看到方法名時就會對方法返回值有個概念。一個與給定問題集合相關聯的主題的集合。
- 現在你能夠了解related_topics表示一個陣列，它裡面存放了一個與給定問題集合相關聯的主題的集合。之前打程式碼裡rt表示什麼非常含糊。
- 使用topic代替之前的t，並用question替換掉q，使得你的程式碼更便於閱讀，因為你不再需要腦補這些變數的用途。現在這些程式碼已然能夠自解釋一切。
條件判斷時使用unless或者否定的表示式
不好的
|
1 2 3 4 5 6 7 8 9 10 11 |
; 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或者肯定的條件表示式之後，上述程式碼就會好懂得多。
|
1 2 3 4 5 6 7 8 9 |
; html-script: false ] # app/models/user.rb class User < ActiveRecord::Base def disabled? !enabled? end end |
|
1 2 3 4 5 6 7 8 9 10 11 |
; 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模型里加的那個。
沒有遵循“命令，不要去詢問”原則
不好的
|
1 2 3 4 5 6 7 8 9 |
; html-script: false ] # app/models/user.rb class User < ActiveRecord::Base def enable! update(enabled: true) end end |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 |
; html-script: false ] # 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 |
這裡的問題是在不恰當的地方出現了控制邏輯。你先判斷了使用者是否是不可用，如果的確不可用，就啟用這個使用者。
好的
比較好的改辦法是把控制邏輯放到enable!方法裡。
|
1 2 3 4 5 6 7 8 9 10 11 |
; html-script: false ] # app/models/user.rb class User < ActiveRecord::Base def enable! if disabled? update(enabled: true) end end end |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 |
; html-script: false ] # 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 |
現在控制器不用關心user需要滿足何種條件才會被啟用。相關的判斷由User模型和enable！方法來處理。
使用複雜的條件
不好的
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 |
; html-script: false ] # 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 |
條件表示式弄的太過複雜了，實際上這裡只想知道一件事：使用者可以刪掉post嗎？
好的
從上面的程式碼我們可以瞭解到，一個使用者需要是post的所有者或者這個使用者是管理員，並且post本身也是可用的，才可以刪除這個post。最好的做法就是，把這些條件抽取成一個日後可以複用的方法。
|
1 2 3 4 5 6 7 8 9 |
; html-script: false ] # app/models/user.rb class User < ActiveRecord::Base def can_destroy_post?(post) post.enabled? && (own_post?(post) || admin?) end end |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 |
; html-script: false ] # 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 |
每當條件表示式裡出現了&&或者||，就應該把它們提取為方法，以備以後複用。