What are best practices of creating database importer in Rails?

Question

I have sample database in GES files and I wrote importer in Ruby.

Sample file: WAR_APO.GES: https://gist.github.com/regedarek/cd0ce73c5d1e14ff9818

I will have several of GES files which I want to move to my data folder in app and I want to process them manually.

1. For most of them logic is the same but the difference are filename, model and attributes hash.
2. I want to also and new cases like: Update when type is U or Delete if is D

How should I refactor that:

class PrototypePharmaceuticImporter
  ID = '00'
  K  = "#{ID}K"
  F  = "#{ID}F"
  FB  = "#{ID}FB"
  D  = "#{ID}D"
  I  = "#{ID}I"
  U  = "#{ID}U"
  E  = "#{ID}E"

  TYPES = [K, F, FB, D, I, U, E]

  ARTICLE_TYPE = 57
  BARCODE = 15
  GROUP_CODE = 1
  GROUP_KEY = 40
  GROUP_NAME = 2
  FORM_CODE = 1
  FORM_KEY = 38
  FORM_NAME = 2
  FORM_SHORT_NAME = 3
  LONGNAME = 28
  PACKAGE_TYPE = 46
  PHARMACY_ONLY = 3
  PRESCRIPTION_ONLY = 47
  PRICE = 2
  WEIGHT = 14

  attr_accessor :file, :k, :f

  def initialize
    @pharmaceutics_file = File.open 'data/PAC_APO.GES', 'r'
    @forms_file = File.open 'data/DAR_APO.GES', 'r'
    @groups_file = File.open 'data/WAR_APO.GES', 'r'
    @k    = []
    @f    = []

    extract_groups_to_database
  end

  def extract_pharmaceutics_to_database
    add       = true
    tmp       = []
    type      = K

    @pharmaceutics_file.each_with_index do |line, i|
      break if (i > 96 * 1000 + 760 + 9)

      _type    = line.strip
      _changed = TYPES.include? _type

      if _changed && i > 0
        case type
        when K then @k << tmp
        when F then @f << tmp
        when FB then @f << tmp
        when I, U, D
          hash = {
            article_type: tmp[ARTICLE_TYPE],
            price: tmp[PRICE],
            weight: tmp[WEIGHT],
            package_type: tmp[PACKAGE_TYPE],
            group_code: tmp[GROUP_KEY],
            form_code: tmp[FORM_KEY],
            name: tmp[LONGNAME],
            barcode: tmp[BARCODE],
            pharmacy_only: tmp[PHARMACY_ONLY],
            prescription_only: tmp[PRESCRIPTION_ONLY]
          }
          Pharmaceutic.create(hash)
        end

        tmp  = []
        type = _type
      end

      tmp << clean(line)
    end
  end

  def extract_forms_to_database
    add       = true
    tmp       = []
    type      = K

    @forms_file.each_with_index do |line, i|
      break if (i > 96 * 1000 + 760 + 9)

      _type    = line.strip
      _changed = TYPES.include? _type

      if _changed && i > 0
        case type
        when K then @k << tmp
        when F then @f << tmp
        when FB then @f << tmp
        when I, U, D
          hash = {
            code: tmp[FORM_CODE],
            name: tmp[FORM_NAME],
            short_name: tmp[FORM_SHORT_NAME]
          }
          PharmaceuticForm.create(hash)
        end

        tmp  = []
        type = _type
      end

      tmp << clean(line)
    end
  end

  def extract_groups_to_database
    add       = true
    tmp       = []
    type      = K

    @groups_file.each_with_index do |line, i|
      break if (i > 96 * 1000 + 760 + 9)

      _type    = line.strip
      _changed = TYPES.include? _type

      if _changed && i > 0
        case type
        when K then @k << tmp
        when F then @f << tmp
        when FB then @f << tmp
        when I, U, D
          hash = {
            code: tmp[GROUP_CODE],
            name: tmp[GROUP_NAME]
          }
          PharmaceuticGroup.create(hash)
        end

        tmp  = []
        type = _type
      end

      tmp << clean(line)
    end
  end

  private

  def clean line
    line.strip
    .gsub(/^[\d]{2}/, '')
    .gsub(/\\[s|S]39/, 'ß')
    .gsub(/\\a25/, 'ä')
    .gsub(/\\A25/, 'Ä')
    .gsub(/\\o25/, 'ö')
    .gsub(/\\O25/, 'Ö')
    .gsub(/\\u25/, 'ü')
    .gsub(/\\U25/, 'Ü')
  end
end

Basically, I want to get rid of duplications in these methods.

Answer 1

Some notes:

• As usual, I'd recommend a more functional approach. It's not a theoretical issue, functional code is more concise, more clear. When I see @k = [] I get the shakes thinking that this variable can (and will) be modified just everywhere in the class (an instance variable is just a nasty global variable -for the scope a class, granted- with a pretty name).

• Also related to FP, your code is hard to abstract because it's written with procedures instead of functions: they don't take values (as arguments) and return values, everything works by side-effects through instance variables. No referential transparency, no idempotence...

• Don't create so many individual constants, group them in hashes.

• File.open 'data/PAC_APO.GES', 'r'. Hardcoded files in classes? mmmm.

• 96 * 1000 + 760 + 9. Magic number, define it as a constant so it has a name.

Answering your particular question about how to avoid repeating code for those 3 methods: whatever is different put it as arguments. If some value depends on something the caller cannot know yet about (the code under when I, U, D), then use an argument block as callback. So it might be written:

def extract_to_database(collection)
  add       = true
  tmp       = []
  type      = K

  collection.each_with_index do |line, i|
    break if (i > 96 * 1000 + 760 + 9)

    _type    = line.strip
    _changed = TYPES.include? _type

    if _changed && i > 0
      case type
      when K then @k << tmp
      when F then @f << tmp
      when FB then @f << tmp
      when I, U, D
        yield(tmp)
      end

      tmp  = []
      type = _type
    end

    tmp << clean(line)
  end
end

Note that I kept your old imperative code (a functional approach would require rewriting the full class), but at least is now DRYed. I'd strongly encourage to try the functional paradigm, it's hard at first but it pays off on the long run. My article on this matter.