I recently had to look over some code that was returning inconsistent results. I was able to fix the code with a few minor changes, but it was still very hard to follow. Take a look:
def audit_self # what level the charts was actually coded for actually_coded = self.cpt_code.code.last.to_i # determine if elements PASS -> 0, FAIL -> 1, or PASS and undercoded -> -1 self.history_result = pass_or_fail_elements( actually_coded, self.history_levels ) self.hpi_result = pass_or_fail_elements( actually_coded, self.hpi_levels ) self.ros_result = pass_or_fail_elements( actually_coded, self.ros_levels ) self.organ_result = pass_or_fail_elements( actually_coded, self.organ_levels ) self.mfs_result = pass_or_fail_elements( actually_coded, self.mfs_levels ) self.mdm_result = pass_or_fail_elements( actually_coded, self.mdm_levels ) if self.any_elements_failed? self.overall_result = 1 elsif self.all_elements_undercoded? self.overall_result = -1 else self.overall_result = 0 end min_level = 5 max_level = 0 for elem in ["history_levels", "mdm_levels", "hpi_levels", "ros_levels", "organ_levels", "mfs_levels"] levels = self.send elem max_level = levels[0] if levels[0] > max_level min_level = levels[1] if levels[1]The first thing I did was to refactor the redundant categories into a hash. This is probably debatable if this is clever or just good DRY practice. This is really up to you - I found Hash#each was up to the task with a couple of send calls.
CATEGORIES = {:history_levels => :history_result=, :hpi_levels => :hpi_result=, :ros_levels => :ros_result=, :organ_levels => :organ_result=, :mfs_levels => :mfs_result=, :mdm_levels => :mdm_result= }So we use Hash#each to run through the hash, run our pass_or_fail method, and then run the setter method to save the result. This should really be refactored further into its own method, but I want to focus more on the Enumerable#inject method for this article.
def audit_self # what level the charts was actually coded for actually_coded = self.cpt_code.code.last.to_i # determine if elements PASS -> 0, FAIL -> 1, or PASS and undercoded -> -1 CATEGORIES.each do |level, result| answer = pass_or_fail_elements(actually_coded, self.send(level)) self.send(result, answer) end #snipped for brevityI refactored the code below to move that ugly for loop into its own method, and used inject instead, which seems much cleaner to me. By moving the code to its own method, writing tests for the logic flow becomes easier. It also makes it easier to track down these type of problems in the future.
self.recommended_code = self.cpt_code.code.chop + minimum_coding_level.to_s self.save end def minimum_coding_level CATEGORIES.inject(0) do |result, level_method| minimum = (self.send level_method[0]).first resultAh, now the good part. Enumberable#inject is a very powerful tool, but if you aren't familiar with it, this code might feel a bit dense or unreadable. Inject allows us to loop through an array or hash - the variable named "result" contains the return value of our block each time. In our case, we are trying to find the highest minimum number. One thing that might look odd in this example is the level_method[0] - since we are using a hash, the key/value pair is set to level_method (as an array) and we just care about the key in this case.
Go forth young padawans and apply these learning to clean up your code and remove those for loops!
*Note:* I do realize this code could be refactored even more, but I really wanted to use this code as an example or how to use Enumerables, not so much about model method refactoring.
