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)
endLine#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("|")
endChargeSlot.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
endChargeSlot.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
endWhile 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