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

评论

180 楼 抛出异常的爱 2008-11-13  

楼上俩人好累啊
如果是好的习惯
你会常常看见.
如果不是好习惯
只会流行一阵子

人是主要矛盾.
当人人都用
那么你写的再晦涩如 "大姨妈"
也不会有什么问题.

如果不常用.你们两费半天力
娇情啥
(北京话 二声 轻声 只是为做个例子)

179 楼 yunhaifeiwu 2008-11-13  

很无聊！

一个代码的写法在不停的纠缠！

我最反感细碎的约束！ 我诚认 isXXX == true是脱裤子放屁，远不如isXXX

!isXXX 比isXXX==false好一点。但是实际上他们达到的目的不一样吗？对性能有显著的影响吗？

我很不喜欢被这种细碎的环节束缚，只有能完成复杂的功能，且性能稳定，我才不管代码有多么的漂亮！

再漂亮的的代码，算法不合理，Bug到处时，不能应对需求变化等，那才叫有屁用！错，连屁都不如！


但我并不反对，在完成了需求时，尽可的让代码漂亮高效一些，只是反感过多的束缚！那种教材主义，教条主义见鬼去吧！

当然，我并不认为李刚的不足，是可以理解的，毕竟号称最佳实践的实践方法书，会给新手带来错误的认识与影响！

但是要老是纠缠于一个语法的不同书写方式（尽管有细微的性能影响），我认为很不值得，很不可取的！

178 楼 icewubin 2008-11-13  

hax 写道

icewubin 写道

hax 写道

没见过 isXXX == true 这样脱裤子放屁的事情

isXXX == true是脱裤子放屁，但是isXXX == false不是，我是这么认为的，不知掉我这样说你能理解我的意思么？

偶知道你是认为 !isXXX 的可读性不如 isXXX == false

反驳意见如下：

A. !是天生的——谁让C的创造者选了 ! ，您就忍忍吧
B. 找个会高亮 ! 的IDE
C. 改为 isNotXXX
D. 改为 !!! isXXX —— 就不相信你看不到！！！

最后一个办法好，实在是高啊，高亮!应该不难的，为啥IDE都不做呢？

再扯一句，null != user和!isXXX都是“判断”前置，如果说null != user不符合阅读习惯，!isXXX也是不符合阅读习惯的，isXXX == false更符合语言习惯。

177 楼 hax 2008-11-13  

icewubin 写道

hax 写道

没见过 isXXX == true 这样脱裤子放屁的事情

isXXX == true是脱裤子放屁，但是isXXX == false不是，我是这么认为的，不知掉我这样说你能理解我的意思么？

偶知道你是认为 !isXXX 的可读性不如 isXXX == false

反驳意见如下：

A. !是天生的——谁让C的创造者选了 ! ，您就忍忍吧
B. 找个会高亮 ! 的IDE
C. 改为 isNotXXX
D. 改为 !!! isXXX —— 就不相信你看不到！！！

176 楼 icewubin 2008-11-13  

hax 写道

没见过 isXXX == true 这样脱裤子放屁的事情

isXXX == true是脱裤子放屁，但是isXXX == false不是，我是这么认为的，不知掉我这样说你能理解我的意思么？

再次声明，我是BS 李×刚的，不是为他维护。

175 楼 hax 2008-11-13  

icewubin 写道

先说第二点，完全赞同，isNotActivated()当然是最佳，我最喜欢用StringUtils.isNotEmpty()函数了，但是不是所有的时候都有这个函数的啊，也不是任何时候都能自己添加这类函数的啊。

然后开始长篇大论说前面那个问题：
null != xxx 虽然是C里的风格，在java中不存在单个=号的误写（语法不允许），但也是可取的，怎么会牺牲可读性呢，请看这个例子：

if (dictionaryFactoryDB.getDictionary(DictEnumCfg.PROPERTY_TIME).getEnumByEnumName(dayContent.getPropertyTime()) != null)

当然你可以说，这句话本身太长，应该分解一下。实际上宽度120字符的话，是不需要分解的，只不过!=null出现在非常靠右的情况了，如果放在左边可读性肯定是更好一点的。

if (null != dictionaryFactoryDB.getDictionary(DictEnumCfg.PROPERTY_TIME).getEnumByEnumName(dayContent.getPropertyTime()))

别人看这段代码的时候，一上来就知道你这个判断的目的是防止非空，带有目的性的预判读代码的效率肯定是高一点的，这和英语考试做阅读的时候先看题目，然后带着问题再阅读的效率肯定更高是一个道理。

为了详细说明，再举一个例子：
...
2.if或for嵌套一多，主题逻辑往右偏，实际屏幕上会出现左边显现大量空白区域，实际很浪费空间，增大自动换行或者右边溢出屏幕的概率。
当然这只是这种策略的附加好处，处理大量空白区域还是有很多其他重构方法的。

if重构的例子，我有时也这样做的。不过这跟前面讨论的问题有点扯远了。
至于左边太长的问题，诚如你所说，还有很多重构方法。

最简单的方法就是赋值给一个短变量。经常要用的话，就应提取一个工具方法。

如果强调预判，则可以搞个 isNull() 或者 isNotNull() 。

俺一向觉得，在进行逻辑运算时，把常量写在左侧，不符合大多数人的思考习惯。
如果这个逻辑运算是特别重要的，则可以炮制一个方法来明确表达之，用不着故意找别扭来提醒大家。

关于 ! 运算，其实我还想说一句，那个不显眼是天生的。要是拿not作为关键字就没有这个问题。

BTW，就readonly所言的要害其实在于
1. ruby的约定不是 isXXX
2. 没见过 isXXX == true 这样脱裤子放屁的事情

补充：就算 null == alonglonglongexpression 有助可读性，那也不符合lg这个case。它如果要说明这样一种最佳实践，就应该拿个恰当的例子出来，并解释解释。但是从他的一贯表现来看，我们是不能指望的。

174 楼 icewubin 2008-11-13  

。。。 写道

icewubin 写道

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

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

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

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

后者挂个!没什么问题，别人看代码漏掉!是别人的事情，说明看代码的人连脑子都不动还指望它看明白代码？省下这点小聪明好好把代码结构搞的通俗易懂点不比这强？

至于前者，感觉没有任何意义。

“把代码结构搞的通俗易懂点”当然比这个强，这个完全赞同的，而且是前提。

在有这个前提下，才说这些阅读友好的代码。写代码的人往往不知道读代码的人的苦，别人越能方便的接手你的代码，对你来说也是好事，而且接手你代码的人能力往往并不是很强的。要有团队协作精神，即使不是接手代码，团队配合中，代码阅读友好也是很有帮助的，这个道理和代码规范（以前的匈牙利命名法）的初衷是一致的。

引用

别人看代码漏掉!是别人的事情，说明看代码的人连脑子都不动还指望它看明白代码？

这话说出来就很没有协作精神，好比写个报告给别人看，当然是越简单，越容易看懂越好，就像你投简历给其他公司人事，肯定不会写的晦涩难懂，考验HR的耐心，一个道理。（插一句，如果主流IDE能够自动高亮加粗“!”号，我马上就支持“!”符号）

这里举的例子都太简单，实际的代码逻辑往往复杂非常多，更多的时候即使你认为的“把代码结构搞的通俗易懂”还是会碰到复杂逻辑，看起来照样是很累的。我认为这些小技巧就属于你说的“把代码结构搞的通俗易懂”的方法中的一种，当然你可以不屑说此类方法比较低级（只是这里没讨论高级的方法而已）。

换种思路理解这个问题，如果一个条件判断比较长（不易阅读，之前的例子都太简单），又不想拆分（拆分也会增加一定的复杂性），那就是不吝惜笔墨加注释，false == 完全可以类比为这段逻辑的注释，没什么不好。

173 楼 下一站，火星 2008-11-13  

<div class='quote_title'>hax 写道</div>
<div class='quote_div'>
<div class='quote_title'>jiajw0426 写道</div>
<div class='quote_div'>技术和文章放在一边不谈,就作者写文章的动机和心态,实在不感恭维!</div>
<br/>不谈技术你来干嘛？你啥心态，你啥动机？ <br/></div>
<p> </p>
<p> 培训托，漫天飞，偶想解密一下JE的那些拖们以及他们的动机，我多少了解一些里面的道道，我今天有时间就写出来，你先等着</p>

172 楼 hax 2008-11-13  

jiajw0426 写道

技术和文章放在一边不谈,就作者写文章的动机和心态,实在不感恭维!

不谈技术你来干嘛？你啥心态，你啥动机？

171 楼 fly_bluewolf 2008-11-13  

最开始入行的时候，我也是买一些什么快速精通之类的书籍，回来一看都是垃圾。国内真正静下心来写书的人很少，中国整个大环境都是浮躁。现在买书都只买老外写的书，人家是真正把自己的书当宝贝孩子在写，那可真是用了心的。

170 楼 Else 2008-11-13  

国内太多这样的书了,我也买过几本,自己还没整明白,就来教学生了,好大一本,又没含金量,这样只会把整个市场搞坏,使那些好的书商和作者受牵连,好的作者写了书赚不到钱,就没人写书了.

对他们的技术水平我持保留意见,他们的目的让人鄙夷,我也很想尊重他们,如果他们也尊重我们这些读者的话

强烈建议抵制这些不良书商和作者的书籍

169 楼 。。。 2008-11-13  

icewubin 写道

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

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

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

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

后者挂个!没什么问题，别人看代码漏掉!是别人的事情，说明看代码的人连脑子都不动还指望它看明白代码？省下这点小聪明好好把代码结构搞的通俗易懂点不比这强？

至于前者，感觉没有任何意义。

168 楼 icewubin 2008-11-13  

hax 写道

icewubin 写道

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

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

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

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

null != xxx 这种将常量写在左边的写法我看也不见得有什么好（难道是怕写出 if(xxx = null)？）反而牺牲了可读性。
至于说!isActivated()不如isActivated() == false，那我看还不如再来个 isNotActivated()。

先说第二点，完全赞同，isNotActivated()当然是最佳，我最喜欢用StringUtils.isNotEmpty()函数了，但是不是所有的时候都有这个函数的啊，也不是任何时候都能自己添加这类函数的啊。

然后开始长篇大论说前面那个问题：
null != xxx 虽然是C里的风格，在java中不存在单个=号的误写（语法不允许），但也是可取的，怎么会牺牲可读性呢，请看这个例子：

if (dictionaryFactoryDB.getDictionary(DictEnumCfg.PROPERTY_TIME).getEnumByEnumName(dayContent.getPropertyTime()) != null)

当然你可以说，这句话本身太长，应该分解一下。实际上宽度120字符的话，是不需要分解的，只不过!=null出现在非常靠右的情况了，如果放在左边可读性肯定是更好一点的。

if (null != dictionaryFactoryDB.getDictionary(DictEnumCfg.PROPERTY_TIME).getEnumByEnumName(dayContent.getPropertyTime()))

别人看这段代码的时候，一上来就知道你这个判断的目的是防止非空，带有目的性的预判读代码的效率肯定是高一点的，这和英语考试做阅读的时候先看题目，然后带着问题再阅读的效率肯定更高是一个道理。

为了详细说明，再举一个例子：

for (User user : users) {
    if (null != user) {
        //xxxx
        //xxxx
        //xxxx
        //xxxx
        //xxxx
        //xxxx
        //xxxx
        //xxxx
        //xxxx
        //xxxx
        if (null != user.getGroup()) {
            //yyyy
            //yyyy
            //yyyy
            //yyyy
            //yyyy
            //yyyy
            //yyyy
            //yyyy
            //yyyy
            //yyyy
        }
    } else {//if。。。 这里加注释是不少C代码的风格，如果不写注释，读到这往往不少人已经记不清这个if是干啥的了
        //aaa这里的代码往往很短
    }
}

如果是我，一定这样改写：

for (User user : users) {
    if (null == user) {
        //aaa这里的代码往往很短
        continue;
    }
    //xxxx
    //xxxx
    //xxxx
    //xxxx
    //xxxx
    //xxxx
    //xxxx
    //xxxx
    //xxxx
    //xxxx
    if (null == user.getGroup()) {
        continue;
    }
    //yyyy
    //yyyy
    //yyyy
    //yyyy
    //yyyy
    //yyyy
    //yyyy
    //yyyy
    //yyyy
    //yyyy
}

虽然这种策略不能解决所有问题，但是大部分情况很好用，逻辑更清楚。
1.可读性好，人在阅读代码的时候，碰到左括号，实际也是一个入栈的过程，碰到右括号出栈，但是人的记性有好有坏，不是每个人能够在看了一大段逻辑代码以后还能记住某个else对应的if到底什么意思，往往要回过去看那个if，而且整个看的过程，脑中必须时刻提醒有这个if前提的存在。改写后，读者很快见到了右括号，说明这一小段逻辑aaa就是处理某种特殊情况的，知道即可，而且很明确剩下的是主要逻辑，大脑中马上就可以把这段特殊逻辑给排空了。

尤其是if或for嵌套一多，这种策略尤其有效。

2.if或for嵌套一多，主题逻辑往右偏，实际屏幕上会出现左边显现大量空白区域，实际很浪费空间，增大自动换行或者右边溢出屏幕的概率。
当然这只是这种策略的附加好处，处理大量空白区域还是有很多其他重构方法的。

167 楼 sunxg 2008-11-13  

java新手，看了很长时间，想了很长时间，偶承认Boolean == Boolean 是脱裤子放屁，没什么用，但实际来说的话有什么实质性的危害吗？？谁给偶说说.....

166 楼 jiajw0426 2008-11-13  

技术和文章放在一边不谈,就作者写文章的动机和心态,实在不感恭维!

165 楼 hax 2008-11-13  

icewubin 写道

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

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

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

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

null != xxx 这种将常量写在左边的写法我看也不见得有什么好（难道是怕写出 if(xxx = null)？）反而牺牲了可读性。
至于说!isActivated()不如isActivated() == false，那我看还不如再来个 isNotActivated()。

164 楼 jaremyismyname 2008-11-13  

强烈支持，李X的书让可爱的初学者浪费了许多金钱，更多的是时间。

163 楼 风花雪月饼 2008-11-13  

icewubin 写道

robbin 写道

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

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

我刚开始和你一样的想法，问题是花了点时间看到后面几页发现他们还真的讨论到在Java中也不能这么写，一样会被鄙视，兜了个圈子还是回到Java的写法了。

现阶段的我绝对是不会鄙视C的写法的（连JS都不鄙视了，鄙视C做啥呀），任何代码和算法都是由小的面向过程式的语法写出来的。

我很好奇，如果是ruby这段到底怎么写才最“最佳实践”？

全面使用ruby的简写方式，比如?之类
ruby最大特点应该就是代码简单了。

我没看过ruby相关的书，也就网上找错误的时候顺便看下别人的代码。现在照样写得啪啦啪啦的。不过里面很多语法我都没记，至今还没记住捕获异常那个单词。。呵呵。。

162 楼 icewubin 2008-11-13  

robbin 写道

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

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

我刚开始和你一样的想法，问题是花了点时间看到后面几页发现他们还真的讨论到在Java中也不能这么写，一样会被鄙视，兜了个圈子还是回到Java的写法了。

现阶段的我绝对是不会鄙视C的写法的（连JS都不鄙视了，鄙视C做啥呀），任何代码和算法都是由小的面向过程式的语法写出来的。

我很好奇，如果是ruby这段到底怎么写才最“最佳实践”？

161 楼 yunhaifeiwu 2008-11-13  

1. 创建一个用户表
这里使用的是sql，而不是Rails最佳实践推荐的migration

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


------------------------------------------------------

如果是ruby语法教材,我可以理解使用sql而不用其他;举用户登陆例子时用明码!

并会购买;(如果要让我给新手推荐,我会告诉他,这仅是讲语法,至于实际应用,你要学其他东西,这本书无能为力!)

如果一本书讲的是软件开发实践,并且号召最佳实践,一看到有明文保存用户密码,而无特别

说明,我会在心里嘲笑一番,接着基本上选择放弃!