师傅领错门，害了你个人 - 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的书给他，让他好好改造去了...

评论

40 楼 jack 2008-11-11  

titans 写道

白痴才这么想！
你重构的代码丢失了业务逻辑，你再好好想想吧。

业务逻辑是什么？ 说下

另外严肃的说，爆粗口是严重不对的。

39 楼 jack 2008-11-11  

Readonly 写道

差点漏了这个，您一定觉得，if (1 == 2) == false 这个代码可读性不错了...

汗，这个代码，如果我看到组员要这样写，是要游街的。

38 楼 jack 2008-11-11  

我来来个稍微简洁点的版本

def pro_activate
  if user = User.find_by_name_and_active_code_and_active(params[:name], params[:active_code], false)
    user.update_attribute(:activated , true)
    flash[:notice] = "恭喜您,您已经成功激活了您的账户!"
  else
    flash[:notice] = "激活失败，您的账户可能不存在或者已经激活，请重新检查你的帐户。"
  end
end

这个函数是用来激活用户的，那么出现特殊情况的，要么就是用户没有注册，尝试进行攻击，或者已经激活过了。所以该写成这样就可以满足使用了。

37 楼 Readonly 2008-11-11  

titans 写道

7. if user != nil && user.is_activated == false
user.is_activated == false ??? 这叫啥代码阿？ 偶的天哪，李刚老师可能是为了保持风格统一，后面果然还有 user.is_activated == true ，这是偶看到搞笑的代码了...

这样的代码可读性不错，当然给初学者看比较合适。这样笑话人家不太地道。

差点漏了这个，您一定觉得，if (1 == 2) == false 这个代码可读性不错了...

36 楼 Readonly 2008-11-11  

titans 写道

什么资深会员，我看你是瞎掰能手，给出一段菜鸟代码，还想作为规范！难道写操作系统教材的人要把真实的操作系统里面进程调度的算法都写全了。我说你不尊重别人就是因为你的无理取闹和没根据的瞎掰。

你不看偶后面的文字么？

引用

而且在实际开发中，作为最佳实践，最后一个else判断完全是不必要的，大家可以想想什么情况下会出现跑到最后一个else？嗯，只有在恶意构造url攻击的情况下，这样我们完全可以改成：
User.find_by_name_and_active_code_and_active(params[:name], params[:active_code], false)

在改成这样的情况下代码应该如何重构还用偶多说么？虽然偶是Ruby/Rails菜鸟，但是这个代码也是呼之逾出阿：

def pro_activate
  user = User.find_by_name_and_active_code_and_active(params[:name], params[:active_code], false)
  if user
    flash[:notice] = "您的账户已经处于激活状态,请勿重复激活!"
  else
    user.update_attribute(:activated , true)
    flash[:notice] = "恭喜您,您已经成功激活了您的账户!"
  end
end

至于你说的明码密码，你随便找一本Rails的书，比如Rails作者DHH的Agile Web Development with Rails，里面都是 t.column :hashed_password, :string

偶承认偶是个rails菜鸟，菜鸟都能从这个书中找出那么多最佳实践的反面教材，这本书不如叫Rails敏捷开发最佳菜鸟鉴定术第一卷...

35 楼 jack 2008-11-11  

playfish 写道

严重怀疑那个titans是个马甲书托。反正不用多理他，浪费时间。何必这么多人跟他一个人过不去。大家眼睛都很亮。

技术是严肃的，并非大家和他过不去

34 楼 jack 2008-11-11  

请问，优点是什么，如果你清楚，请诉说一，二

33 楼 playfish 2008-11-11  

严重怀疑那个titans是个马甲书托。反正不用多理他，浪费时间。何必这么多人跟他一个人过不去。大家眼睛都很亮。

32 楼 jack 2008-11-11  

哦，抱歉，看错了。编辑掉。。。

31 楼 抛出异常的爱 2008-11-11  

估计不能说话了.....
----------------------------
鸟的...算错数了......
本板要50点....

30 楼 jack 2008-11-11  

引用

什么资深会员，我看你是瞎掰能手，给出一段菜鸟代码，还想作为规范！

光说不练呢。。。 没啥意思。

从偶尔泄露的代码看，这边ruby水平感觉不错的，快客王，gigix，其他的人印象不深。

兄弟，来点实际点的代码吧

29 楼 yehs220 2008-11-11  

titans 写道

2. 用户表中直接用明码保存用户的密码
请各位作实际开发新手注意：任何一个应用都不应该犯这样愚蠢的错误

人家是写书，不是教你开发一个权限系统，这样说有些过分。

3.标示用户是否激活的字段名叫is_actived
这不符合rails的最佳实践写法，对于boolean类型的数据，应该省略前面的is_，ActiveRecord会自动加个?号，映射成actived?

数据库命名无可厚非。难道不懂Ruby的人不可以为Ruby程序员设计数据库了吗？这样不太厚道。

5. 设置用户的默认激活为false：@user.is_activated = false
Rails的理念是COC和DRY，这种初始默认值的设定应该在创建数据库定义的时候指定，而且默认的boolean都是false，这里的赋值完全是多此一举。

呵呵，数据库设计这忘记了为Ruby程序员定义初值怎么办？

7. if user != nil && user.is_activated == false
user.is_activated == false ??? 这叫啥代码阿？ 偶的天哪，李刚老师可能是为了保持风格统一，后面果然还有 user.is_activated == true ，这是偶看到搞笑的代码了...

这样的代码可读性不错，当然给初学者看比较合适。这样笑话人家不太地道。

8. 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

这样的代码也不见得就可以作为规范，IF嵌套层次多了，可读性不好。

尊重别人也是尊重自己。

此人可疑，应该刚注册不久，回的几个帖子都是本帖...

28 楼 阳光晒晒 2008-11-11  

上次的事....
太牛了....
比看电视剧过瘾多了.

27 楼 liuqiang 2008-11-11  

2,3 如果这本书免费的话 我可以原谅 可惜书的价格既然定到79.8 原来不懂Ruby的人也可以为Ruby程序员设计数据库 托大?在跳大仙吧

5 已经说了很清楚了 请先去理解什么是nil vs null再来抬杠 空对空的扯淡 没有任何意义
7 同2,3 请记住这本书卖79.8
8 2个IF嵌套层次也叫多？您来做个示范如何？

26 楼 Readonly 2008-11-11  

titans 写道

人家是写书，不是教你开发一个权限系统，这样说有些过分。

这本书的名字叫"最佳"实践，按照你的说法，是不是应该叫"纸上谈兵"实践更为合适一些？

titans 写道

数据库命名无可厚非。难道不懂Ruby的人不可以为Ruby程序员设计数据库了吗？这样不太厚道。

哦，那么你的厚道观点是什么，莫非是：难道不懂Ruby的人不可以为Ruby程序员写书吗？

后面的偶也懒得一一反驳了。

titans 写道

尊重别人也是尊重自己。

尊重知识，是对智慧的向往，尊重生命，是对生活的热爱
尊重这种书和这种作者？偶的脑袋被压路机压过后说不定有这个可能...

25 楼 抛出异常的爱 2008-11-11  

titans 写道

2. 用户表中直接用明码保存用户的密码
请各位作实际开发新手注意：任何一个应用都不应该犯这样愚蠢的错误

人家是写书，不是教你开发一个权限系统，这样说有些过分。
(下略....)

楼上还是用java吧ruby这东西不合适当教材.

24 楼 风花雪月饼 2008-11-11  

还好，我学JAVA的时候是用的那书上画个动物的那种。

学RAILS，毛书都没用，就开始写代码去了。哈哈哈

23 楼 hetylei 2008-11-11  

确实挺搞笑 ，可以改名了
<<Ruby on Rails敏捷开发 之 山寨最佳实践>>

22 楼 冉翔 2008-11-11  

<div class='quote_title'>下一站，火星 写道</div>
<div class='quote_div'>
<div class='quote_title'>范三山 写道</div>
<div class='quote_div'>
<div class='quote_title'>下一站，火星 写道</div>
<div class='quote_div'>
<p>分析的不错!</p>
<p> </p>
<p>《Web开发大全——Ruby on Rails版》（<a href='http://product.dangdang.com/product.aspx?product_id=20319929'>http://product.dangdang.com/product.aspx?product_id=20319929</a>）这本书貌似不错，驴子以为如何？</p>
</div>
<p>那是兔子的说......</p>
<p> </p>
<p>这本书作者写着  “编委会” 著 是什么意思？</p>
</div>
<p><br/>这本书是gigix做的序 ， 5个作者起码有4个在JE，大家伙基本都认识，看目录的话，感觉编排的内容很丰富</p>
<p> </p>
<p> </p>
</div>
<p> </p>
<p>恩呐，4个中其中俩在俺们单位。</p>

21 楼 Ashela 2008-11-11  

只要看李刚出书的范围之广，就知道他书一定不行