乐者为王

Do one thing, and do it well.

重构Rails代码

在以前写的博文部署应用到Heroku时的问题里有这么一段话:

股票功能需要导入交割单文件,因为导入后的文本文件不再使用,可以把上传路径由public/uploads改为tmp,这样就避免了Heroku不能写文件的问题。

下面是那时候写的导入代码:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
class StocksController < ApplicationController
  UPLOADS_DIRECTORY = File.join("#{Rails.root}", "public/uploads")

  def import
    filename = upload_file(params[:file])
    lines = File.readlines("#{UPLOADS_DIRECTORY}/#{filename}")
    lines.each do |line|
      # do something
    end
    redirect_to stocks_url
  end

  protected
    def upload_file(file)
      if !file.original_filename.empty?
        @filename = get_file_name(file.original_filename)
        Dir.mkdir("#{UPLOADS_DIRECTORY}") unless Dir.exist?("#{UPLOADS_DIRECTORY}")
        File.open("#{UPLOADS_DIRECTORY}/#{@filename}", "wb") do |f|
          f.write(file.read)
        end
        return @filename
      end
    end

    def get_file_name(filename)
      if !filename.nil?
        # does not support chinese filename?
        Time.now.strftime("%Y%m%d%H%M%S") + '.txt'
      end
    end
end

这里的实现方法是先将上传文件保存到服务器上应用的public/uploads目录中,然后再读取和处理。

其实根本不需要写的这么复杂,因为导入的文件被读取一次后就不再使用了。所以在当时写代码的时候一直有这样的想法,如果能直接获得上传文件的数据,那么就不需要再另外去写保存和读取文件的代码了。

事实也是如此。通过表单提交的file数据会首先在服务器上形成临时文件,这时其实可以通过临时文件的路径来读取上传文件的数据。

根据该想法重构后的代码如下:

1
2
3
4
5
6
7
8
class StocksController < ApplicationController
  def import
    lines = File.readlines(params[:file].tempfile.to_path.to_s)
    lines.each do |line|
      # do something
    end
    redirect_to stocks_url
  end

重构后的代码果然清爽多了,不过还是有改进的空间。

作为控制器,controller只是负责接收request,并返回response。而具体的业务逻辑,则应该交由model去完成。下面是依照该理念再次重构后的代码:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
class StocksController < ApplicationController
  def import
    Stock.import(params[:file])
    redirect_to stocks_url
  end
end

class Stock < ActiveRecord::Base
  def self.import(file)
    lines = File.readlines(file.tempfile.to_path.to_s)
    lines.each do |line|
      # do something
    end
  end
end

Comments