最近看到一位Ruby On Rails新手写的代码,简直是惨不忍睹,问了一下他竟然是用李刚写的那本<<Ruby on Rails敏捷开发最佳实践>>作为入门材料的,偶真是faint,啥也不多说了,为了让更多的Ruby On Rails的新手避免走弯路,偶觉得很有必要来评论一下这本书,以china pub上下载的第15章样章为例子:
[url]http://www.china-pub.com/39348[/url]
这章是讲一个注册用户通过邮件注册激活的例子,偶们来看看所谓的最佳实践实际是如何成为绝佳反面教材:
1. 创建一个用户表
这里使用的是sql,而不是Rails最佳实践推荐的migration
2. 用户表中直接用明码保存用户的密码
请各位作实际开发新手注意:任何一个应用都不应该犯这样愚蠢的错误
3. 标示用户是否激活的字段名叫is_actived
这不符合rails的最佳实践写法,对于boolean类型的数据,应该省略前面的is_,ActiveRecord会自动加个?号,映射成actived?
4. User模型的校验功能用errors.add这样手工的方法
最佳实践是使用Validation DSL,除非ActiveRecord提供的DSL不能满足你的特殊校验需求,否则毫无必要自己手工处理
就算你有很特殊的校验需求,在这段代码里面也应该使用errors.add_to_base,而不是用errors.add_base,跟一个空白的字符串
5. 设置用户的默认激活为false:@user.is_activated = false
Rails的理念是COC和DRY,这种初始默认值的设定应该在创建数据库定义的时候指定,而且默认的boolean都是false,这里的赋值完全是多此一举
让我们先休息和回顾一下,上面提到的5个绝佳反面教材竟然是出现在一个章节中的一个小段:15.4.1基本注册功能,错误之密集真是令人咋舌。
如果你和偶一样,还能忍受下去,恭喜你,你具备了给烂代码作Code Review的基本要求:[color=red]耐力[/color]。
偶们来继续往下看:
6. 看看发送邮件的代码
@sent_on指定的是邮件头信息中的Date,默认就是使用当前时间,这里的赋值也是多此一举
@headers指定的是邮件头信息的hash,这里也是也是多此一举
结合之前的问题5,李刚老师您这是用无用代码来充书本厚度么?
7. if user != nil && user.is_activated == false
user.is_activated == false ??? 这叫啥代码阿? 偶的天哪,李刚老师可能是为了保持风格统一,后面果然还有 user.is_activated == true ,这是偶看到搞笑的代码了...
8. pro_activate Action的代码写得及其冗长
综合前面的错误,偶想李刚可能压根不懂一些ruby和rails的惯用法,偶好为人师一下,符合ruby风格的代码应该是这样:
而且在实际开发中,作为最佳实践,最后一个else判断完全是不必要的,大家可以想想什么情况下会出现跑到最后一个else?嗯,只有在恶意构造url攻击的情况下,这样我们完全可以改成:
User.find_by_name_and_active_code_and_active(params[:name], params[:active_code], false)
在之后的pro_login action里面的代码就不多说了,和前面一样,也有user != nil && user.is_activated == false这样搞笑的代码出现。
看完了这个短短5页的样章中有那么多的错误,偶明白了为什么那个新手写的代码是如此的惨不忍睹,虽然俗话说,师傅领进门,修行在个人,但是也要晓得:师傅领错门,害了你个人
最后推荐了另外几本Ruby和Rails的书给他,让他好好改造去了...
[url]http://www.china-pub.com/39348[/url]
这章是讲一个注册用户通过邮件注册激活的例子,偶们来看看所谓的最佳实践实际是如何成为绝佳反面教材:
1. 创建一个用户表
这里使用的是sql,而不是Rails最佳实践推荐的migration
2. 用户表中直接用明码保存用户的密码
请各位作实际开发新手注意:任何一个应用都不应该犯这样愚蠢的错误
3. 标示用户是否激活的字段名叫is_actived
这不符合rails的最佳实践写法,对于boolean类型的数据,应该省略前面的is_,ActiveRecord会自动加个?号,映射成actived?
4. User模型的校验功能用errors.add这样手工的方法
最佳实践是使用Validation DSL,除非ActiveRecord提供的DSL不能满足你的特殊校验需求,否则毫无必要自己手工处理
就算你有很特殊的校验需求,在这段代码里面也应该使用errors.add_to_base,而不是用errors.add_base,跟一个空白的字符串
5. 设置用户的默认激活为false:@user.is_activated = false
Rails的理念是COC和DRY,这种初始默认值的设定应该在创建数据库定义的时候指定,而且默认的boolean都是false,这里的赋值完全是多此一举
让我们先休息和回顾一下,上面提到的5个绝佳反面教材竟然是出现在一个章节中的一个小段:15.4.1基本注册功能,错误之密集真是令人咋舌。
如果你和偶一样,还能忍受下去,恭喜你,你具备了给烂代码作Code Review的基本要求:[color=red]耐力[/color]。
偶们来继续往下看:
6. 看看发送邮件的代码
@sent_on = Time.now
@headers = {}
@sent_on指定的是邮件头信息中的Date,默认就是使用当前时间,这里的赋值也是多此一举
@headers指定的是邮件头信息的hash,这里也是也是多此一举
结合之前的问题5,李刚老师您这是用无用代码来充书本厚度么?
7. if user != nil && user.is_activated == false
user.is_activated == false ??? 这叫啥代码阿? 偶的天哪,李刚老师可能是为了保持风格统一,后面果然还有 user.is_activated == true ,这是偶看到搞笑的代码了...
8. pro_activate Action的代码写得及其冗长
综合前面的错误,偶想李刚可能压根不懂一些ruby和rails的惯用法,偶好为人师一下,符合ruby风格的代码应该是这样:
def pro_activate
user = User.find_by_name_and_active_code(params[:name], params[:active_code])
if user
if user.actived?
flash[:notice] = "您的账户已经处于激活状态,请勿重复激活!"
else
user.update_attribute(:activated , true)
flash[:notice] = "恭喜您,您已经成功激活了您的账户!"
end
else
flash[:notice] = "激活失败!"
end
end
而且在实际开发中,作为最佳实践,最后一个else判断完全是不必要的,大家可以想想什么情况下会出现跑到最后一个else?嗯,只有在恶意构造url攻击的情况下,这样我们完全可以改成:
User.find_by_name_and_active_code_and_active(params[:name], params[:active_code], false)
在之后的pro_login action里面的代码就不多说了,和前面一样,也有user != nil && user.is_activated == false这样搞笑的代码出现。
看完了这个短短5页的样章中有那么多的错误,偶明白了为什么那个新手写的代码是如此的惨不忍睹,虽然俗话说,师傅领进门,修行在个人,但是也要晓得:师傅领错门,害了你个人
最后推荐了另外几本Ruby和Rails的书给他,让他好好改造去了...