师傅领错门，害了你个人 - ruby/rails新手避免入错门

最近看到一位Ruby On Rails新手写的代码，简直是惨不忍睹，问了一下他竟然是用李刚写的那本<<Ruby on Rails敏捷开发最佳实践>>作为入门材料的，偶真是faint，啥也不多说了，为了让更多的Ruby On Rails的新手避免走弯路，偶觉得很有必要来评论一下这本书，以china pub上下载的第15章样章为例子：
http://www.china-pub.com/39348

这章是讲一个注册用户通过邮件注册激活的例子，偶们来看看所谓的最佳实践实际是如何成为绝佳反面教材：
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的基本要求：耐力。

偶们来继续往下看:
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的书给他，让他好好改造去了...

评论

80 楼 Readonly 2008-11-11  

gigix 写道

if(user.getActivated() == true) {
  return true;
} else {
  return false;
}

这样的你见过没？

这个确实要比李刚书中写的要强上一点
偶之前看过比较强的有：霹雳啪啦一堆代码，计算出一个值，偶好不容易看懂了那一堆代码，但是......发现那个值在下面的代码压根没有用到。事后询问那写代码的哥们，他很不以为然地回了偶一句：忘记删了 

79 楼 yehs220 2008-11-11  

buaawhl 写道

一叶孤鸿 写道

if(false) {
// 这里一大段代码
}

这样的你见过没？


这个确实很NB的说

这种代码方便白盒测试。
改成 true 就运行，改成 false 就不运行。

没错，我偶尔也这么干

78 楼 ddd 2008-11-11  

gigix 写道

ddd 写道

gigix 写道

if(false) {
// 这里一大段代码
}

这样的你见过没？

也许是注释掉代码的意思

说到注释，故事就更多了

一个class我随便拖一拖，两千多行，中间两三百行一大段是直接注释掉的
问这些人，为什么不删掉，说是不能删，再问为什么，一个人说不知道，另一个人说留着以后可能需要参考
参考你参考clearcase好不好，版本控制要来干嘛的呢？

还有，方法名不好好写，参数列表就是什么a1、a2、a3…到上面再去拉一堆注释，说这个方法是干嘛的，每个参数是干嘛的。有这工夫你直接写在方法签名里不好？

还有，一个方法拉出三四百行，中间注释出几大块。把注释直接就变成一个新方法的名字有那么难吗？用的还是Eclipse…

说得不好听点，会写注释的程序员我就见得不太多。见过太多程序员，写一行注释就是一个反模式。看他们的代码就搜注释，一行行注释看过去全都是毛病。

单就这一个案例（用if(false){}注释掉代码）来讲，在linux内核中也能见到
#if 0
一段代码
#endif
用c比较习惯的人对这种注释方法都能接受。
可能你见到的那个人从C过来的？还带着C的习惯？
-------add------------
不过这种不好看的东西可能也是被bsd内核维护者笑话的原因之一。

77 楼 gigix 2008-11-11  

ddd 写道

gigix 写道

if(false) {
// 这里一大段代码
}

这样的你见过没？

也许是注释掉代码的意思

说到注释，故事就更多了

一个class我随便拖一拖，两千多行，中间两三百行一大段是直接注释掉的
问这些人，为什么不删掉，说是不能删，再问为什么，一个人说不知道，另一个人说留着以后可能需要参考
参考你参考clearcase好不好，版本控制要来干嘛的呢？

还有，方法名不好好写，参数列表就是什么a1、a2、a3…到上面再去拉一堆注释，说这个方法是干嘛的，每个参数是干嘛的。有这工夫你直接写在方法签名里不好？

还有，一个方法拉出三四百行，中间注释出几大块。把注释直接就变成一个新方法的名字有那么难吗？用的还是Eclipse…

说得不好听点，会写注释的程序员我就见得不太多。见过太多程序员，写一行注释就是一个反模式。看他们的代码就搜注释，一行行注释看过去全都是毛病。

76 楼 ddd 2008-11-11  

gigix 写道

if(false) {
// 这里一大段代码
}

这样的你见过没？

也许是注释掉代码的意思

75 楼 buaawhl 2008-11-11  

一叶孤鸿 写道

if(false) {
// 这里一大段代码
}

这样的你见过没？


这个确实很NB的说

这种代码方便白盒测试。
改成 true 就运行，改成 false 就不运行。

74 楼 一叶孤鸿 2008-11-11  

if(false) {
// 这里一大段代码
}

这样的你见过没？


这个确实很NB的说

73 楼 playfish 2008-11-11  

QiaoDuanni 写道

weiqingfei 写道ahuaxuan 写道神作啊,神作,读"李老师"的书,如饮甘泉,又有如清风拂面,又有一种让人可以恍然大悟的感觉,原来代码还可以这样写:
if(user.isActivated() == false)
我大澈大悟,我有理由相信这种写法是某培训学校的最佳实践


"李老师"从java,到ruby,无所不知,无所不晓,我本人非常期待"李老师"的下一本神作"python on django敏捷开发最佳实践", 或者groovy on grails 敏捷开发最佳实践,或者"李老师"还会其他的东西

凡是是"李老师"写的书,都是好书,凡是"李老师"做的事,都是好事.




我想看李刚版的《倚天屠龙记》
余秋雨 写道
我想，比‘大’字等级更高的是‘老’字，一个人先成‘大人’才能成为‘老人’，那么，既然我已经做了大半辈子的‘老师’，那就后退一步叫叫‘大师’也可以吧。

我建议还是叫"李大师"

不能叫李大师，已经有李宏志了。会混淆的。这评论再下去就都水了。还是就此打住的好。。免得又出现什么门事件来

72 楼 gigix 2008-11-11  

Readonly 写道

就算不熟ruby，也没有人在java里面这样用：
if(user.isActivated() == false)把...

这种代码我看得多了

if(user.getActivated() == true) {
  return true;
} else {
  return false;
}

这样的你见过没？

if(false) {
// 这里一大段代码
}

这样的你见过没？

我都见过。这世界上不会写程序的程序员之牛逼远超过我之前最牛逼的想象──当然现在我又被颠覆了一次，不会写程序的程序员也可以写书教别人写程序。

71 楼 QiaoDuanni 2008-11-11  

weiqingfei 写道

ahuaxuan 写道

神作啊,神作,读"李老师"的书,如饮甘泉,又有如清风拂面,又有一种让人可以恍然大悟的感觉,原来代码还可以这样写:
if(user.isActivated() == false)
我大澈大悟,我有理由相信这种写法是某培训学校的最佳实践


"李老师"从java,到ruby,无所不知,无所不晓,我本人非常期待"李老师"的下一本神作"python on django敏捷开发最佳实践", 或者groovy on grails 敏捷开发最佳实践,或者"李老师"还会其他的东西

凡是是"李老师"写的书,都是好书,凡是"李老师"做的事,都是好事.

我想看李刚版的《倚天屠龙记》

余秋雨 写道

我想，比‘大’字等级更高的是‘老’字，一个人先成‘大人’才能成为‘老人’，那么，既然我已经做了大半辈子的‘老师’，那就后退一步叫叫‘大师’也可以吧。

我建议还是叫"李大师"

70 楼 tangjunchf 2008-11-11  

请推荐几本好书啊！

69 楼 weiqingfei 2008-11-11  

ahuaxuan 写道

神作啊,神作,读"李老师"的书,如饮甘泉,又有如清风拂面,又有一种让人可以恍然大悟的感觉,原来代码还可以这样写:
if(user.isActivated() == false)
我大澈大悟,我有理由相信这种写法是某培训学校的最佳实践


"李老师"从java,到ruby,无所不知,无所不晓,我本人非常期待"李老师"的下一本神作"python on django敏捷开发最佳实践", 或者groovy on grails 敏捷开发最佳实践,或者"李老师"还会其他的东西

凡是是"李老师"写的书,都是好书,凡是"李老师"做的事,都是好事.

我想看李刚版的《倚天屠龙记》

68 楼 ahuaxuan 2008-11-11  

神作啊,神作,读"老师"的书,如饮甘泉,又有如清风拂面,又有一种让人可以恍然大悟的感觉,原来代码还可以这样写:
if(user.isActivated() == false)
我大澈大悟,我有理由相信这种写法是某培训学校的最佳实践


"老师"从java,到ruby,无所不知,无所不晓,我本人非常期待"老师"的下一本神作"python on django敏捷开发最佳实践", 或者groovy on grails 敏捷开发最佳实践,或者"老师"还会其他的东西

凡是是"老师"写的书,都是好书,凡是"老师"做的事,都是好事.

"老师"千秋万代,一统江湖

67 楼 ycmhn 2008-11-11  

upheart 写道

if user != nil &amp;&amp; user.is_activated == false 佩服，就是java代码也不能这么写啊李刚好像什么都精通奥，在书店好像还看到他的一本flex的书

貌似我偶尔会写出 if(xxx===false){}
这样的啊。。。。晕了

66 楼 logo 2008-11-11  

上个月(10月份)李副教授在上海签名售书呢，我总觉得这个帖子出来的太晚了。现在烂书这么多，多一本不多少一本不少，就怕还当什么培训学校的教学总监，那才真是“造福一方百姓”阿！

65 楼 ddd 2008-11-11  

tanleihaoren 写道

啊！惨了。见证“刚门事件了”！希望不要发展成双方的人生攻击！

这得给对号了。

64 楼 fujohnwang 2008-11-11  

Readonly 写道

fujohnwang 写道

引用

if user != nil && user.is_activated == false

ruby 不熟，这行代码在ruby里面应该用什么套路？！

if user && user.is_activated

就算不熟ruby，也没有人在java里面这样用：
if(user.isActivated() == false)把...

呵呵，我想我是想知道前面那个部分...

63 楼 Readonly 2008-11-11  

fujohnwang 写道

引用

if user != nil && user.is_activated == false

ruby 不熟，这行代码在ruby里面应该用什么套路？！

if user && user.is_activated

就算不熟ruby，也没有人在java里面这样用：
if(user.isActivated() == false)把...

62 楼 jiachengxi38 2008-11-11  

一句话，，责任在何方。。。

61 楼 fujohnwang 2008-11-11  

引用

if user != nil && user.is_activated == false

ruby 不熟，这行代码在ruby里面应该用什么套路？！