Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Refactoring + code review of some code I saw in a blog

Refactored some code from a blog about service objects: https://www.codewithjason.com/code-without-service-objects/


Guessing that ZirMed837PFile::File.new(appointments).write(file.path) is supposed to be something like this:

file = ZirMed837PFile::File.from_queued
file.write(file.filename)

Otherwise the .from_queued and #filename methods don't make much sense. But the filename method probably shouldn't be an instance method, given that it's passed in to #write as an argument.


If we follow the appointments from .from_queued down into to_s and then into Line#initialize, we see that each one has .charges called on it, which appears to be an ActiveRecord::Relation. This will have an N+1 query. Looking around more, there are a bunch of these, there will be a lot of N+1 queries (and probably worse in some places)

def ZirMed837PFile::File.from_queued
  new Appointment
    .joins(:charges)
    .includes(
      :patient, # Just a guess, it wasn't clear where this came from
      :physician,
      location: :address,
      charges: [:diagnosis_code, :cpt_code],
    )
    .merge(Charge.queued)
end

Line#fields_with_charges would be better private since its only called by Line#to_s.


Line#fields_with_charges appears to do a fairly expensive operation, and it gets called 213 times by #to_s, so it would be good to memoize it. Then again, it's a hash, and the block just asks for keys, so you could also get away with not memoizing it if you only call it once, which you can do with Hash#to_proc (using a range to avoid needing to add 1 in the block):

def to_s
  (1..TOTAL_NUMBER_OF_FIELDS).map(&fields_with_charges).join("|")
end

ChargeSlot.charge_slot_offsets can be simpler if you use three dots, which will exclude the final index

def self.charge_slot_offsets
  0...NUMBER_OF_CHARGE_SLOTS
end

ChargeSlot.charge_slots does a select that makes it look like some of the charges could be empty, but it only does that b/c it's iterating over a range that's potentially larger than the array. You can use head instead. It doesn't fit the best with the code as it currently is, b/c the code is more interested in the indexes than the charges. But after refactoring those things out (passing it the charge instead of the array and index), it will work nicely.


ChargeCollection#charge_slots winds up limiting the number of slots to 6 (because of that above charge_slots_offsets), but ChargeCollection#unique_diagnosis_codes does not. So if there were enough charges with different diagnosis codes, it would write into other fields. It should probably be like unique_diagnosis_codes.take(NUMBER_OF_DIAGNOSIS_SLOTS)


I18n.l is probably a hidden bug (meaning it could unexpectedly change the date format in prod) because it renders localized dates. I'd check the spec, it probably specifies a format.


You don't need to explicitly call to_s on the hash values, join will call it for you.


Note that the joining in ZirMed837PFile::File#to_s means there will not be a newline on the last line. Seemed potentially incorrect.


With a hash, you can call fetch and it will invoke the block if the key DNE, so you can refactor place_of_service_code

Also use inspect in the error message instead of typing quotes in it. Because if there were quotes in type.to_s, then the displayed value would not be correctly escaped. This will also prevent nil from looking like an empty string.

The values in location.type seem like an unwise choice, BTW. (1) it'll be painful to call an ActiveRecord column type (I assume it's ActiveRecord), b/c AR will think it's single table inheritance. (2) The values look like they're for presentation, which makes them ill suited as a data representation, might convert it to an enum, and look up the presentation string through I18n.

def place_of_service_code
  PLACE_OF_SERVICE_CODES.fetch @appointment.location.type do |type|
    raise KeyError, "Unable to find place of service code for location type #{type.inspect}"
  end
end

While reading it, I started tweaking it a bit. This is what I wound up at. I didn't really go into the LineSerializer because it was unclear where some of that data was coming from. Note that I have no way of running any of this, so who knows how correct it is.

# Opening the file allows it to write each line as it is generated so it doesn't
# have to build up a potentially massive string in memory.
File.open Rails.root.join('tmp', 'charges.txt'), "w" do |file|
  ZirMed837PFile::Document.new(Appointment.all).write file
end

require 'csv'
module ZirMed837PFile
  class Document
    def initialize(appointments_relation)
      @appointments = appointments_relation
        .joins(:charges)
        .includes(  # `includes` fixes the N+1 queries
          :patient, # Just a guess that this is here, it wasn't clear where it came from
          :physician,
          location: :address,
          charges: [:diagnosis_code, :cpt_code],
        )
        .merge(Charge.queued)
    end

    def to_s
      write "" # Should work b/c `write` uses `<<`, which both IO and String implement
    end

    def write(stream)
      # Receive the stream so you can write it into, eg, a socket,
      # or some specific part of a currently open file.
      each_line { |line| stream << line.to_s }
      stream
    end

    def each_line
      return to_enum __method__ unless block_given?
      appointments.each { |appt| yield Line.new appt }
    end
  end
end


require 'csv'
module ZirMed837PFile
  class Line
    TOTAL_NUMBER_OF_FIELDS    = 213
    DIAGNOSIS_CODES_OFFSET    = 65  # Starting it 1 earlier b/c document seems to count from 1 instead of 0
    NUMBER_OF_DIAGNOSIS_SLOTS = 12  # I added this b/c first date of service is @ 77, which is 12 more than 65
    NUMBER_OF_CHARGE_SLOTS    = 6   # What happens if you have more than 6 charges? I'd expect that happens a lot
    DATE_FORMAT               = '%Y%m%d' # Not sure what it should be, but almost certainly not `I18n.l`

    def initialize(appointment)
      @appointment = appointment
    end

    def to_s
      # Swapped the join("|") with CSV b/c it was just so sus to me, e.g.
      # there's user submitted data in there, what if the user enters a pipe?
      # that said, it's a total guess.
      CSV.generate_line to_a, col_sep: '|'
    end

    def to_a
      (1..TOTAL_NUMBER_OF_FIELDS).map &to_h
    end

    def to_h
      other_fields.merge diagnosis_code_fields, charges_fields
    end

    private

    def other_fields
      @other_fields ||= LineSerializer.new(@appointment).fields
    end

    # Maybe sort these to put the most expensive ones first? Someone might get denied their insurance claim b/c the aspirin was charge 1 and the MRI was charge 7 and thus omitted, making the bill look sus
    def charges
      @charges ||= appointment.charges.to_a
    end

    def diagnosis_code_pointers
      @diagnosis_code_pointers ||= charges
        .map(&:diagnosis_code).map(&:code).uniq
        .take(NUMBER_OF_DIAGNOSIS_SLOTS) 
        .each.with_index(1).to_h
    end

    def diagnosis_code_fields
      @diagnosis_code_fields ||= diagnosis_code_pointers.to_h do |code, offset|
        [DIAGNOSIS_CODES_OFFSET+offset, code]
      end
    end

    def charges_fields
      @charges_fields ||= charges
        .take(NUMBER_OF_CHARGE_SLOTS)
        .flat_map.with_index { |charge, offset|
          [ [offset+77,  charge.date_of_service.strftime(DATE_FORMAT)],        # date of service
            [offset+101, charge.cpt_code.code],                                # procedure code
            [offset+107, charge.modifier],                                     # procedure code modifier
            [offset+113, diagnosis_code_pointers[charge.diagnosis_code.code]], # diagnosis code pointer
            [offset+119, charge.cpt_code.fee_per_unit],                        # charge
            [offset+125, charge.units],                                        # units
            [offset+167, charge.cpt_code.ndc_code],                            # ndc code
          ]
        }.to_h
    end

  end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment