为什么我要重构一个view模板

重构前.html.erb

<!-- 
  这是一个来自真实项目的view模板，
  在这里我做了一些简化后，使得阅读更加容易一些。
  实际环境中大约是2倍行数。

  程序执行时没有错误，一切正常。
  美中不足的是，当用户没有登录时，也会显示一个
  <span class="message-count"></span> 标签
  给用户造成有消息的错觉

  所以我打算优化一下体验，优化之后见下个文件
-->

<%
    if current_user
        comments_count = current_user.notifications_queue[:comments] + current_user.notifications_queue[:messages] + current_user.notifications_queue[:notifications]
    end
%>
<ul class="weike-menu">
    <li>...</li>
    <li>...</li>
    <li>
      <a href="/mine/messages" >
        <i class="iconfont">&#349;</i> 我的私信 
        <span class="message-count"><%= comments_count %></span>
      </a>
    </li>
</ul>

重构前，试图优化，当没有消息的时候隐藏消息数字.htmll.erb

<!-- 
  这是优化后的、有问题的模板。
  当用户没有登录时，“comments_count > 0“ （19行）就会异常：
  
  “undefined method `>' for nil:NilClass”
-->

<%
    if current_user
        comments_count = current_user.notifications_queue[:comments] + current_user.notifications_queue[:messages] + current_user.notifications_queue[:notifications]
        comments_count ||= 0    #<------------- 增加的语句
                                #犯的错误：我应该把这句写在if之外
    end
%>
<ul class="weike-menu">
    <li>...</li>
    <li>...</li>
    <li>
      <% if comments_count > 0 %> <!--- <------------ added -->
        <a href="/mine/messages" >
          <i class="iconfont">&#349;</i> 我的私信 
          <span class="message-count"><%= comments_count %></span>
        </a>
      <% end %>
    </li>
</ul>

重构后1：view.html.erb

<!-- 
  这是重构后的模板，去掉了开头的 if 判断
  判断逻辑放到 application_helper 中，见下面两个文件
-->

<ul class="weike-menu">
    <li>...</li>
    <li>...</li>
    <li>
      <% if current_unread_count > 0 %> <!-- <--这里就可以放心用了 -->
        <a href="/mine/messages" >
          <i class="iconfont">&#349;</i> 我的私信 
          <span class="message-count"><%= current_unread_count %></span>
        </a>
      <% end %>
    </li>
</ul>

重构后2: application_helpers.rb

# in app/helpers/application_helper.rb
def current_unread_count
  return 0 unless current_user
  current_user.unread_message_count
end

重构后3: user.rb

# in app/models/user.rb
def unread_message_count
  queue = notifications_queue
  return 0 if notifications.blank?
  queue[:comments] + queue[:messages] + queue[:notifications]
end

我的小结：

1. 嵌入在 view 模板中逻辑，很容易造成维护上的风险，尽量放到helper中。
2. 如果非要用 if， 要考虑条件不满足的情况，不要明知一个变量是nil，还输出它。其他人可能会需要优化体验，可能直接在旁边对这个变量做一些判断。
3. 这些细微到不足为道的隐患、代码的坏味道，日积月累，就会毁了软件

early return 用的很多啊，我比较喜欢写成这样：

if notifications.blank?
  0
else
  queue[:comments] + queue[:messages] + queue[:notifications]
end

early return 只在可以有效减少缩进层级的时候用，毕竟代码长了之后它们很容易被无视掉