rails重构笔记-将代码写的更好一点：更少的代码，更清晰的结构，更好的封装

1. 批量查询的问题
   原代码
   

       friends = []
       Network.transaction do
         fids["ids"].each do |fid|#所有跟随的用户的ID号
           nw = Network.find_by_uuid(fid)
           friends << nw.user if nw
         end
       end

   从数据库表中，取得一系列用户的ID号，需要批量查询两张表，但实际上，我只需要一张表的数据
   所以，需要做两个方面的改变：第一，使用 in 查询， 第二，不使用关联表，代码如下
   

    Network.find(:all,:select => "user_id", :conditions => ["uuid in (?)", fids]).map(&:user_id)

   
   
   
2. if  else 太多，逻辑混乱
   原代码
   

   def weibo_profile(token, secret)
       type = "weibo"
       if self.send(type.to_sym)
         unless Rails.cache.read('w_user')
           oauth = Weibo::OAuth.new(Weibo::Config.api_key, Weibo::Config.api_secret)
           oauth.authorize_from_access(self.send(type.to_sym).token, self.send(type.to_sym).secret)
           w_user = Weibo::Base.new(oauth).verify_credentials
           Rails.cache.write('w_user', w_user, :expires_in => 15.minutes)
           w_user
         else
           Rails.cache.read('w_user')
         end
       else
         if  token && secret
           oauth = Weibo::OAuth.new(Weibo::Config.api_key, Weibo::Config.api_secret)
           oauth.authorize_from_access(token, secret)
           w_user = Weibo::Base.new(oauth).verify_credentials
           Rails.cache.write('w_user', w_user, :expires_in => 15.minutes)
           w_user
         else
           nil
         end
       end
     end

   
   方法比较长，逻辑非常混乱，一定要重构。
   1，将oauth提取出来，放到network model 里
   

   def self.get_oath(token, secret)
       oauth = Weibo::OAuth.new(Weibo::Config.api_key, Weibo::Config.api_secret)
       oauth.authorize_from_access(token, secret)
       oauth
   end

   
   network_model.rb 文件
   
   2，去掉缓存的逻辑
   3， 使用try方法，去掉nil的判断
   最后代码如下
   

    def weibo_profile(token=nil, secret=nil)
       type = "weibo"
       token = token || self.send(type.to_sym).try(:token)
       secret = secret || self.send(type.to_sym).try(:secret)
       oauth = Network.get_oath(token, secret)
       Weibo::Base.new(oauth).verify_credentials
     end

   
   接着重构代码！
   如上的代码，有一个小问题，就是每次都会到远程服务器取数据，显然不可行，所以，完全有必要定义一个 实例变量，将数据保存在实例变量中，修改后的代码如下
   

    def weibo_profile(token=nil, secret=nil)
       type = "weibo"
       token = token || self.send(type.to_sym).try(:token)
       secret = secret || self.send(type.to_sym).try(:secret)
       if @np
         @np
       else
         oauth = Network.get_oath(token, secret)
         @np = Weibo::Base.new(oauth).verify_credentials
       end
     end

   
   
3. controller层逻辑太多
   这种情况比较常见，比如我们要搜索一些视频，这样，我们必须根据搜索条件构建复杂的conditions
   

   case date
           when "week"
             start_time = 1.week.ago.to_s(:db)
           when "month"
             start_time = 1.month.ago.to_s(:db)
           when "day"
             start_time = Time.now.at_beginning_of_day.to_s(:db)
           when "all"
             start_time = current_user.created_at
           end
           end_time = Time.now.to_s(:db)
           conditions = ["user_id = ? AND private in (?) AND created_at > ? AND created_at < ? ",
             current_user.id, state, start_time, end_time]
           if !last.blank? && last != 0
             conditions.first << "AND id < ?"
             conditions << last
           end
           unless title.blank?
             conditions.first << "AND title LIKE ?"
             conditions << "%#{title}%"
           end

   
   很显然，这段代码非常的长，不便于维护，而且，不能重用。所以，完全有必要将这段代码放到model层
   model video 代码片段：
   

     ["day", "week", "month"].each do |timezone|
       case timezone
       when "week"
         start_time = 1.week.ago.to_s(:db)
       when "month"
         start_time = 1.month.ago.to_s(:db)
       else
         start_time = Time.now.at_beginning_of_day.to_s(:db)
       end
       named_scope timezone.to_sym, :conditions => ["created_at > ? and created_at < ?", start_time, Time.now.to_s(:db)]
     end

   controller中的代码如下：
   

   if ["day","month", "week"].include?(date)
             @videos = Video.archived.send(date.to_sym).find(:all,:conditions => conditions, :limit => num,
               :order => "created_at DESC")
           else
             @videos = Video.archived.find(:all,:conditions => conditions, :limit => num,
               :order => "created_at DESC")
           end

   
   还是有很多问题，不过比以前版本的代码要好很多了