在模型的实例方法里，本来不需要的时候使用了“self.”
不好的
# app/models/user.rb
class User < ActiveRecord::Base
def full_name
"#{self.first_name} #{self.last_name}"
end
end
这段代码并不复杂但是里面并不需要使用“self.”。把“self.”去掉会使代码更简洁且不影响可用性。
好的
在模型里，只有在实例方法里需要赋值时，才会用到“self.”，否则通篇的“self.”只会徒增代码复杂度。
# app/models/user.rb
class User < ActiveRecord::Base
def full_name
"#{first_name} #{last_name}"
end
end
使用条件表达式并且返回了条件
不好的
# 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
这段代码的问题在于：在不需要的地方添加了控制语句。
好的
有种更简便的处理方式也能达到同样效果
# app/models/user.rb
class User < ActiveRecord::Base
def full_name
name || "No name"
end
end
简单来说这段代码会在name不为false或nil时将其返回，否则返回”No name”.
使用得当的话，||和&&这些操作符会对提升你的代码品质提供巨大助力。
在视图层使用行内样式
不好的
<!-- app/views/projects/show.html.erb --> ... <h3 style="font-size:20px;letter-spacing:normal;color:#95d60a;line-height:100%;margin:0;font-family:'Proxima Nova';"> SECRET PROJECT </h3> ...
这里我们只列出一个标签，所有的样式都写在了标签里。现在，请设想一下，如果所有的标签都接收行内样式。这会把你的HTML变得和其难度，除此之外，每当你需要引入另一个同样的h3元素时，将不得不把同样代码照搬一边，造成冗余。
好的
// app/assets/stylesheets/application.css
.project-title {
font-size: 20px;
letter-spacing: normal;
color: #95d60a;
line-height: 100%;
margin: 0;
font-family:'Proxima Nova';
}
<!-- app/views/projects/show.html.erb --> ... <h3 class="project-title"> SECRET PROJECT </h3> ...
现在我么可以复用样式了，并且HTML的可读性也有所提高。
注意：这只是个简单的范例，实际应用时你应该把CSS拆分成多个小文件，并通过application.css来加载这些文件。另外只有在email模板里，才会用到行内样式。
在视图层使用JavaScript
不好的
<!-- app/views/questions/show.html.erb -->
...
<textarea rows="4" cols="50" class='wysihtml5'>
Insert your question details here.
</textarea>
...
<script>
$(document).ready(function(){
$('textarea.wysihtml5').wysihtml5({
"font-styles": true, //Font styling, e.g. h1, h2, etc. Default true.
"emphasis": true, //Italics, bold, etc. Default true.
"lists": true, //(Un)ordered lists, e.g. Bullets, Numbers. Default true.
"html": false, //Button which allows you to edit the generated HTML. Default false.
"link": true, //Button to insert a link. Default true.
"image": true, //Button to insert an image. Default true.
"color": true //Button to change color of font. Default true.
});
});
<script>
这里的逻辑和特定页面耦合在一起，导致代码不可复用。
好的
Rails里面有专门用于组织和存放javascript代码的地方：“app/assets/javascripts/”。
// app/assets/javascripts/application.js
...
$(document).ready(function(){
$('textarea.wysihtml5').wysihtml5({
"font-styles": true, //Font styling, e.g. h1, h2, etc. Default true.
"emphasis": true, //Italics, bold, etc. Default true.
"lists": true, //(Un)ordered lists, e.g. Bullets, Numbers. Default true.
"html": false, //Button which allows you to edit the generated HTML. Default false.
"link": true, //Button to insert a link. Default true.
"image": true, //Button to insert an image. Default true.
"color": true //Button to change color of font. Default true.
});
});
...
<!-- app/views/questions/show.html.erb --> ... <textarea rows="4" cols="50" class='wysihtml5'> Insert your question details here. </textarea> ...
现在我们可以在view层任何地方用这段代码了。只需要页面上有一个带有wysihtml5这个class的textarea，刚才的那段js就会被执行。
注意：这只是个简单的范例，实际应用时需要考虑是否需要把你的JavaScript拆分成若干小的文件，并通过application.js来加载这些文件。另外，如果你使用的是CoffeeScript而非JavaScript，请坚持不要把CoffeeScript与普通JavaScript在一起混写。
调用方法时把另一个方法的调用作为参数
不好的
# app/services/find_or_create_topic.rb
class FindOrCreateTopic
...
def self.perform(user, name)
find(user, sluggify(name)) || create(user, name)
end
...
end
这段代码里调用了find方法并传入了2个参数，首参数为user，第二个参数则是直接调用了sluggify这个方法并把name作为参数传给sluggify。你也许会有疑问，这么写有什么问题吗？我明明完全能够看懂这段代码呀。是的，代码也许不难理解，但是每次到这里你都需要自己做一点脑筋转换，而这正是我一直极力想要避免的。
好的
避免需要脑筋转换的一个比较有效的办法就是：使用有意义的变量名。
# app/services/find_or_create_topic.rb
class FindOrCreateTopic
...
def self.perform(user, name)
slug = sluggify(name)
find(user, slug) || create(user, name)
end
...
end
这样做可以避免脑筋转换。换用含义明确的变量名之后，每当你再调用find方法，就会知道find接受一个user和一个slug做参数。
没有使用类来隔离Rake Tasks
不好的
# lib/tasks/recalculate_badges_for_users.rake
namespace :users do
desc "Recalculates Badges for Users"
task recalculate_badges: :environment do
user_count = User.count
User.find_each do |user|
puts "#{index}/#{user_count} Recalculating Badges for: #{user.email}"
if user.questions_with_negative_votes >= 1 || user.answers_with_negative_votes >= 1
user.grant_badge('Critic')
end
user.answers.find_each do |answer|
question = answer.question
next unless answer && question
days = answer.created_at - question.created_at
days_count = (days / 1.day).round
if (days_count >= 60) && (question.vote_count >= 5)
grant_badge('Necromancer') && return
end
end
end
end
end
这个rake task有问题多多。最主要的问题是，这个rake太长了而且不好测试。代码写的初一看也很难理解。你只好相信这个task的确是为用户重新计算奖章系统的，因为它上面描述就这么写的。
好的
解决这个问题最好的办法就是，把业务逻辑挪到一个特定的类里面。所以，让我们新建个类来搞定它吧。
# app/services/recalculate_badge.rb
class RecalculateBadge
attr_reader :user
def initialize(user)
@user = user
end
def grant_citric
if grant_citric?
user.grant_badge('Critic')
end
end
def grant_necromancer
user.answers.find_each do |answer|
question = answer.question
next unless answer && question
if grant_necromancer?(answer, question)
grant_badge('Necromancer') && return
end
end
end
protected
def grant_citric?
(user.questions_with_negative_votes >= 1) ||
(user.answers_with_negative_votes >= 1)
end
def days_count(answer, question)
days = answer.created_at - question.created_at
(days / 1.day).round
end
def grant_necromancer?(answer, question)
(days_count(answer,question) >= 60) &&
(question.vote_count >= 5)
end
end
# lib/tasks/recalculate_badges_for_users.rake
namespace :users do
desc "Recalculates Badges for Users"
task recalculate_badges: :environment do
user_count = User.count
User.find_each do |user|
puts "#{index}/#{user_count} Recalculating Badges for: #{user.email}"
task = RecalculateBadge.new(user)
task.grant_citric
task.grant_necromancer
end
end
end
如你所见，现在这个rake task可读性要好的多，而且还可以单独测试每一种批准徽章的方法。除此以外我么也可以在有需要时复用这个类。当然这段代码只是点到即止，诸位可以再做进一步优化。
没有遵循Sandi Metz的规则
- 每个类代码不可以超过100行
- 每个方法代码不可以超过5行
- 方法参数不可以超过4个，hash项也包括在内
- 控制器之可以初始化一个对象。而且视图层只可以使用一个实例变量，并且只可以在这个对象上调用方法（@object.collaborator.value这种是不可以的）。
更多关于Sandi Metz的规则请移步至thoughtbot,参阅Sandi Metz’ Rules For Developers这篇博文。