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

评论

160 楼 icewubin 2008-11-13  

Readonly 写道

bcccs 写道

单论上面一句，他说的没错啊。ror和java高手出来说说嘛。

这个不需要高手，偶就可以告诉你：这种if boolean == boolean的代码无论出现在李刚的书里面还是出现在哪个所谓著名的开源代码里面，都是属于新手作品。如果你被XX作者或者XX著名开源的光环所迷惑，进而开始怀疑自己的判断，很好，脑白金广告或者那些男科医院的广告就是专门为你拍的。

说真的，我认为现在的新手反而写不出这种代码。

159 楼 robbin 2008-11-12  

icewubin 写道

Readonly 写道

titans 写道
7. if user != nil &amp;&amp; user.is_activated == false user.is_activated == false ??? 这叫啥代码阿？ 偶的天哪，李刚老师可能是为了保持风格统一，后面果然还有 user.is_activated == true ，这是偶看到搞笑的代码了... 这样的代码可读性不错，当然给初学者看比较合适。这样笑话人家不太地道。 差点漏了这个，您一定觉得，if (1 == 2) == false 这个代码可读性不错了... 

就这点来说楼主有点过激了吧，我不熟悉ruby，但是如果是java的话，我会这么写的：

if (null != user && false == user.isActivated())

而且是最近一年才改用这种写法的。

就算作者李刚你再怎么鄙视（我也鄙视他），但是他不会连这个都不知道的吧。这样写都是故意的，而且是推荐的，因为“!”在阅读代码的时候很容易漏掉的，尤其是在看别人的代码的时候。

我也来晚了，帖子太长，没有仔细看。

我就说一句吧： 如果你不懂ruby代码，不了解ruby的代码风格，最好还是退散一下的好，否则出口就让行家笑话。这就好比有人用C语言的过程式语法写Java，一样会被你鄙视的。

158 楼 reeze 2008-11-12  

   1. @sent_on = Time.now 
   2. @headers = {} 

不过这里啊。rails自动生成的代码里有这样一段：

def send_all(sent_at = Time.now)
    subject    'SystemMailer#send_all'
    recipients ''
    from       ''
    sent_on    sent_at
   
    body       :greeting => 'Hi,'
end

不知是何意。意在提醒我们可以设置发送时间么。
哦。顺便问一下 recipients的问题。
我想同时给很多人发邮件  recipients "[e-mail], [e-mail]"
如果[e-mail] 用户不存在的话，会让整个邮件都发不出。。Unkown user,很不爽。不知道有没有办法～

157 楼 yunhaifeiwu 2008-11-12  

这个作者,却是有点不对!


写教材时,通常会用最简单(但不符各种规则)的例子,说明某个用法.

如:我给自已的笔记,写struts2笔记时,牵扯到持久层时,会直接用内存数据代替.
在写spring security笔记时,大多数例子的密码会用明文.

当然,作笔书与写教材是有区别的.当教材,出现了非实践中最佳原则时,应当明确指出,以免误了没有经验的新手.这是作者严重不该的地方.

156 楼 yehs220 2008-11-12  

火星叔叔马丁 写道

有新东方的多了 而且那个是陕西...
田xx不是广东吗？

从他又是删图片，又是删博客来看，估计...

155 楼 yehs220 2008-11-12  

<div class='quote_title'>火星叔叔马丁 写道</div>
<div class='quote_div'>
<div class='quote_title'>yehs220 写道</div>
<div class='quote_div'>
<div class='quote_title'>Readonly 写道</div>
<div class='quote_div'>
<div class='quote_title'>yehs220 写道</div>
<div class='quote_div'>果然是一家的<br/>http://ltian.iteye.com/blog/213966 最后那张图片右下角</div>
<br/>偶很好奇那篇博客的图片右下角有什么，他刚刚编辑掉了显示 最后修改：15 分钟前<br/>他和谁是一家的？李刚？<br/></div>
<br/>呵呵，他也够快，预料中，截屏了<br/></div>
<p> </p>
<p>我来晚了 什么情况 v教 主也是李刚一伙的?</p>
<p> </p>
</div>
<p> </p>
<p>看截图，xxx“新东方”...</p>

154 楼 icewubin 2008-11-12  

一叶孤鸿 写道

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

这样的你见过没？


这个确实很NB的说

见过，是测试过程中产生的，原本因为某种原因可能是这样

if(true)

的，然后应该用注释的没用，可能是IDE不佳，就直接改false了。

当然你看到的代码的作者可能真的是犯错误了。

153 楼 icewubin 2008-11-12  

gigix 写道

Readonly 写道

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

这种代码我看得多了

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

这样的你见过没？

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

这样的你见过没？

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

原因同楼上我的帖子，例子过激了，原例子是没什么问题的，但是你扩展的两个例子是有问题的（废话，当然是有问题的反例）。

其中扩展例子中的第一个我会这么些（废话，一定这么写）：

return user.getActivated();

但是，但是，如果稍微修改一下（反例）：

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

我一定会这么写的：

return false == user.getActivated();

152 楼 icewubin 2008-11-12  

Readonly 写道

titans 写道
7. if user != nil &amp;&amp; user.is_activated == false user.is_activated == false ??? 这叫啥代码阿？ 偶的天哪，李刚老师可能是为了保持风格统一，后面果然还有 user.is_activated == true ，这是偶看到搞笑的代码了... 这样的代码可读性不错，当然给初学者看比较合适。这样笑话人家不太地道。 差点漏了这个，您一定觉得，if (1 == 2) == false 这个代码可读性不错了... 

就这点来说楼主有点过激了吧，我不熟悉ruby，但是如果是java的话，我会这么写的：

if (null != user && false == user.isActivated())

而且是最近一年才改用这种写法的。

就算作者李刚你再怎么鄙视（我也鄙视他），但是他不会连这个都不知道的吧。这样写都是故意的，而且是推荐的，因为“!”在阅读代码的时候很容易漏掉的，尤其是在看别人的代码或者别人看你的代码的时候。

151 楼 logo 2008-11-12  

我算看明白了，Readonly这招叫做“打草惊蛇”呀！ 通过点评李刚的书，彻底的把潜伏在JavaEye的一批书托们统统一网打尽。 高，实在是高！ 

150 楼 yehs220 2008-11-12  

Readonly 写道

yehs220 写道

果然是一家的
http://ltian.iteye.com/blog/213966 最后那张图片右下角

偶很好奇那篇博客的图片右下角有什么，他刚刚编辑掉了显示 最后修改：15 分钟前
他和谁是一家的？李刚？

呵呵，他也够快，预料中，截屏了

149 楼 下一站，火星 2008-11-12  

<div class='quote_title'>xpf7622 写道</div>
<div class='quote_div'>    我觉得此文偏颇.代码写的稍不符合规范就不是高手？Rails给Boolean类型加个？我看更不舒服。 <br/>    依你眼光，Office软件源代码肯定有数不清的弊病，微软搞Office开发的都是混饭的。 <br/><br/>   </div>
<p> </p>
<p> 自己代码写的都不符合规范，还出什么书，这么简单的道理，有什么好争的。</p>
<p> </p>
<p>如果不出书，就是把代码写的再恶心，人家吃饱了撑的才去管他， 而且你愿意花79块钱买一堆垃圾代码看吗</p>

148 楼 xpf7622 2008-11-12  

    我觉得此文偏颇.代码写的稍不符合规范就不是高手？Rails给Boolean类型加个？我看更不舒服。
    依你眼光，Office软件源代码肯定有数不清的弊病，微软搞Office开发的都是混饭的。

  

147 楼 qinglangee 2008-11-12  

qinglangee 写道

我不是李刚的学生，只是今天刚好看了commons的代码，站错队被人叫哥哥了，唉

我都没听说过这个人

146 楼 qinglangee 2008-11-12  

Readonly 写道

qinglangee 写道
apach commons项目里的代码
StringUtils.isBlank(String str)
Java代码 for (int i = 0; i &lt; strLen; i++) {
      if ((Character.isWhitespace(str.charAt(i)) == false)) {
          return false;
      }
  }
          for (int i = 0; i &lt; strLen; i++) {
            if ((Character.isWhitespace(str.charAt(i)) == false)) {
                return false;
            }
        }

apache commons util的代码是出奇的烂，难道别人写得烂，你就以此为借口摆烂，不思进取，还有脸号称最佳实践？

而且偶去翻了一下StringUtils的源代码，在众人的批判下，起码他还没有那么厚的脸皮，很早知错就改：
http://svn.apache.org/viewvc/xml/xindice/trunk/java/src/org/apache/xindice/util/StringUtilities.java?view=markup

Java代码 public static boolean isBlank(String str) {
      for (int i = 0; i &lt; str.length(); i++) {
          if (!Character.isWhitespace(str.charAt(i))) {
              return false;
          }
      }
        return true;
  }
      public static boolean isBlank(String str) {
        for (int i = 0; i &lt; str.length(); i++) {
            if (!Character.isWhitespace(str.charAt(i))) {
                return false;
            }
        }

        return true;
    }

偶很好奇，你是不是李刚培训班出来的学生？

你引用是怎么引用的？我引的别人代码都没格式了
我不是李刚的学生，只是今天刚好看了commons的代码，站错队被人叫哥哥了，唉

145 楼 下一站，火星 2008-11-12  

<div class='quote_title'>Readonly 写道</div>
<div class='quote_div'>
<div class='quote_title'>yehs220 写道</div>
<div class='quote_div'>果然是一家的 <br/>http://ltian.iteye.com/blog/213966 最后那张图片右下角</div>
<br/>偶很好奇那篇博客的图片右下角有什么，他刚刚编辑掉了显示 最后修改：15 分钟前 <br/>他和谁是一家的？李刚？ <br/></div>
<p> </p>
<div class='quote_title'>我 写道</div>
<div class='quote_div'>大家注意，那个titans 就是ltian，难怪名字这么相似。</div>
<p> </p>
<p>titans的扯淡在这一页还能看到：<a href='/post/733362?page=3'>http://www.iteye.com/post/733362?page=3</a> ，建议作为永久封存 </p>
<p><a href='http://ltian.iteye.com/blog/266303'>http://ltian.iteye.com/blog/266303</a>  再看看这个人多有霸气</p>
<p> </p>
<p> </p>

144 楼 Readonly 2008-11-12  

yehs220 写道

果然是一家的
http://ltian.iteye.com/blog/213966 最后那张图片右下角

偶很好奇那篇博客的图片右下角有什么，他刚刚编辑掉了显示 最后修改：15 分钟前
他和谁是一家的？李刚？

143 楼 yehs220 2008-11-12  

果然是一家的
http://ltian.iteye.com/blog/213966 最后那张图片右下角

142 楼 wosmvp 2008-11-12  

引用

mochow  2008-08-04   回复
我觉得吧，大家也别太较劲，培训机构首先是商业性质的，这意味着它首先以赚钱为第一目标。提高知名度显然对它赚钱更为有利，所以和它纠缠就等于在帮它。不理它，让它沉寂才是正确的做法，再说了，有跟它纠缠的时间还不如好好想想怎么把手头的工作做好。

“ 都别装了”同学妄图通过在javaeye一系列的所见所得来揭露些什么，依我看，除了增加八卦话题给管理员带来麻烦之外没有别的好处。在这个世界上，别想着去救别人，让他们自己挣扎，有能力的人迟早会自己挣扎出来的，没能力的人救也救不出来。难道在他们自己的网站上不是有人觉得javaeye很恶心么？物以类聚，有些人天生就适合去这种机构进行入门培训，要让他们天天来javaeye问入门问题，还不把所有人都烦死。

没有了魔鬼，天使还有存在的意义么。javaeye要想活的更好，发展更好，真还需要大量的这类机构的存在，有他们的存在，一部分人在智商上会很有一种优越感

大家还是暂停的好……
时间那是来辩论这个的……

141 楼 下一站，火星 2008-11-12  

<div class='quote_title'>ltian 写道</div>
<div class='quote_div'>
<div class='quote_title'>gigix 写道</div>
<div class='quote_div'><br/>qinglangee 写道apach commons项目里的代码 <br/>StringUtils.isBlank(String str) <br/>Java代码 for&amp;nbsp;(int&amp;nbsp;i&amp;nbsp;=&amp;nbsp;0;&amp;nbsp;i&amp;nbsp;&amp;lt;&amp;nbsp;strLen;&amp;nbsp;i++)&amp;nbsp;{&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;if&amp;nbsp;((Character.isWhitespace(str.charAt(i))&amp;nbsp;==&amp;nbsp;false))&amp;nbsp;{&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;return&amp;nbsp;false;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;}&amp;nbsp;&amp;nbsp;}&amp;nbsp;&amp;nbsp;        for (int i = 0; i &amp;lt; strLen; i++) { <br/>            if ((Character.isWhitespace(str.charAt(i)) == false)) { <br/>                return false; <br/>            } <br/>        } <br/><br/>哥哥…这是在循环里，表达式为true的时候是不能直接返回滴… <br/></div>
<br/>你真的很聪明，看出了这一点，不过人家强调的不是逻辑是写法。 <br/>for (int i = 0; i &amp;lt; strLen; i++) <br/>　{ <br/>         if (!(Character.isWhitespace(str.charAt(i)))) <br/>　　　　　{ <br/>                return false; <br/>            } <br/>   } <br/>呵呵，楼主认为我改写的模式采购专业，那个＝＝flase是个不得了的问题，让人笑掉大牙的问题，而那个老兄举个官方例子只是说说明挑类似这样程序写法毛病的人非常无聊。</div>
<p> </p>
<p> 拿官方例子扯淡，＝＝flase本来就有逻辑问题，连逻辑都不通，还扯什么写法问题！</p>
<p> </p>
<p> </p>