Comparing verbosity of algorithmic Ruby class

Question

At a high level, I'm writing a Ruby class that compares components on two differing versions of the same JSON structure, which in turn decides whether or not said elements should be updated on a PDF file. Every component has a 'componentUuid' attribute which is used to identify components. The comparison is between the first and last versions of the JSON, and has the following business logic:

1. If an element exists in the first JSON, and doesn't in the last, then it's been deleted.
2. If an element exists in both versions, but it's offset has changed, then it's been updated.
3. Updating an element means to delete it, and create it with new attributes.

My implementation of said business logic was using a hash where I assign each component's componentUuid to it for the first JSON, and then use that as reference for diffing against the last JSON. I later refactored it to make the code more 'clean'. However, I can't help but think that it's increased the complexity of the code, and reads far less linearly to me at least.

Any suggestions on how I can better refactor the code to be more performant, or clean would greatly be appreciated.

Problem domain notes:

• We only care about row_element components
• A form_component is a component that wraps a row_element

Original Code:

class SetPdfFieldsToUpdate
  def call(document_content, first_document_content)
    latest_form_json   = document_content.form_json
    original_form_json = first_document_content.form_json

    @pdf_field_uuids_to_remove = []
    @page_to_fd_components_hash = {}

    original_form_json_component_uuid_lookup = {}

    original_form_json["pages"].each_with_index do |original_form_page, original_form_page_index|
      original_form_page["floating_components"].each do |component|
        if component["element_origin"] == "pdf"
          component_uuid = component["field_rows"][0]["row_elements"][0]["componentUuid"]

          original_form_json_component_uuid_lookup[component_uuid] = component["field_rows"][0]["row_elements"][0]
        end
      end

      latest_form_page = latest_form_json["pages"][original_form_page_index]

      latest_form_page["floating_components"].each do |component|
        if component["element_origin"] == "pdf"
          current_component = component["field_rows"][0]["row_elements"][0]
          component_uuid    = current_component["componentUuid"]

          if original_form_json_component_uuid_lookup.key?(component_uuid)
            origin_component_offset  = original_form_json_component_uuid_lookup[component_uuid]["offset"]
            current_component_offset = current_component["offset"]

            unless origin_component_offset == current_component_offset
              recreate_pdf_component_with_new_attributes(current_component, component_uuid, original_form_page_index)
            end

            original_form_json_component_uuid_lookup.except!(component_uuid)
          end
        end
      end
    end

    @pdf_field_uuids_to_remove.concat(original_form_json_component_uuid_lookup.keys)
  end

  private

  def recreate_pdf_component_with_new_attributes(component, component_uuid, page_index) 
    @pdf_field_uuids_to_remove.push(component_uuid)
    @page_to_fd_components_hash[page_index.to_s].push(component)
  end
end

Refactored Code:

class SetPdfFieldsToMoveAndRemove
  def call(document_content, first_document_content)
    latest_form_json = document_content.form_json
    original_form_json = first_document_content.form_json

    @page_to_fd_components_hash = {}
    @pdf_field_uuids_to_remove = []

    @original_pdf_components_by_uuid_lookup = {}

    original_form_json["pages"].each_with_index do |original_form_page, original_form_page_index|
      update_pdf_component_lookup_with_page(original_form_page)

      latest_form_page_by_original_match = get_matching_page_from_latest_form_json(
                                             latest_form_json,
                                             original_form_page_index
                                           )

      latest_form_page_by_original_match["floating_components"].each do |form_component|
        if is_element_origin_pdf?(form_component)
          row_element_attr = get_row_element_from_form_component(form_component)
          row_element_uuid = row_element_attr["componentUuid"]

          if @original_pdf_components_by_uuid_lookup.key?(row_element_uuid)
            check_component_for_offset_shift(row_element_attr, original_form_page_index)
            # check_component_for_type_toggle(current_component)
            # ... add methods for checking if component should be recreated
          end
        end
      end
    end

    delete_pdf_components_not_in_latest_form
  end

  private

  def update_pdf_component_lookup_with_page(form_page)
    form_page["floating_components"].each do |form_component|
      if is_element_origin_pdf?(form_component)
        row_element_attr = get_row_element_from_form_component(form_component)
        row_element_uuid = row_element_attr["componentUuid"]

        @original_pdf_components_by_uuid_lookup[row_element_uuid] = row_element_attr
      end
    end
  end

  def get_row_element_from_form_component(form_component)
    form_component["field_rows"][0]["row_elements"][0]
  end

  def is_element_origin_pdf?(form_component)
    form_component["element_origin"] == "pdf"
  end

  def get_matching_page_from_latest_form_json(latest_form_json, original_form_page_index)
    latest_form_json["pages"][original_form_page_index]
  end

  def check_component_for_offset_shift(component, page_index)
    component_uuid           = component["componentUuid"]
    origin_component_offset  = @original_pdf_components_by_uuid_lookup[component_uuid]["offset"]
    current_component_offset = component["offset"]

    unless origin_component_offset == current_component_offset
      recreate_pdf_element_with_new_attributes(component_uuid, component, page_index)
    end

    update_lookup_for_existing_component_in_latest_form(component_uuid)
  end

  def update_lookup_for_existing_component_in_latest_form(component_uuid)
    @original_pdf_components_by_uuid_lookup.except!(component_uuid)
  end

  def recreate_pdf_element_with_new_attributes(component_uuid, component, page_index)
    @pdf_field_uuids_to_remove.push(component_uuid)
    @page_to_fd_components_hash[page_index.to_s].push(component)
  end

  def delete_pdf_components_not_in_latest_form
    components_uuids_remaining = @original_pdf_components_by_uuid_lookup.keys
    @pdf_field_uuids_to_remove.concat(components_uuids_remaining)
  end
end

Answer 1

The big thing I would do here is add some classes to make this more object oriented and easier to read and understand. When I have super complex code that I have trouble understanding and I want to refactor it for readability, I sit down with what the code is supposed to be doing and then write what I wish the code would look like to do that. From there, I start adding or changing whatever needs to happen in the code base to make the code I wish I had the code I have. Doing so with this code (assuming I understand it all correctly), the call method ended up becoming something like:

  def call(document_content, first_document_content)
    latest_pages = pages_from document_content
    original_pages = pages_from first_document_content

    original_pages.zip(latest_pages).each do |original_page, latest_page|
      original_page.row_elements.each do |row_element|
        # BR1: If an element exists in the first JSON, and doesn't in the last,
        # then it's been deleted
        original_page.delete(row_element) unless latest_page[row_element]

        # BR2: If an element exists in both versions, but it's offset has
        # changed, then it's been updated.
        unless row_element.offset == latest_page[row_element].offset
          original_page.update(latest_page[row_element])
        end
      end
    end
  end

and now I just start going through and implementing the methods required to reach that, so first line in this method, I need to add pages_from:

private
def pages_from(document)
  document.form_json['pages'].map { |page| Page.new(page) }
end

And that makes up the totality of that file, now I just need to start going through the rest of the method and adding the classes and methods on those classes to achieve this goal:

class Row
  # expose the underlying data, so we can modify it later as needed
  attr_reader :row_element

  def initialize(row_element)
    @row_element = row_element
  end

  def uuid
    @row_element['componentUuid']
  end

  def offset
    @row_element['offset']
  end
end

class Page
  def initialize(page)
    @page = page
    # still not happy with this block, but overall the code is a lot nicer,
    # so it can stay, for now
    @row_elements = page['floating_components'].map do |fc|
      next unless fc['element_origin'] == 'pdf'

      Row.new(fc['field_rows'][0]['row_elements'][0])
    end.compact.group_by(&:uuid).transform_values(&:first)

    @fields_to_remove = []
    @fields_to_update = []
  end

  def [](row)
    @row_elements[row.uuid]
  end

  def row_elements
    return to_enum(:row_elements) unless block_given?

    @row_elements.each_value do |row|
      yield row
    end
  end

  def delete(row)
    @fields_to_remove << row
  end

  def update(row)
    delete row

    @fields_to_update << row
  end
end

Not being able to run some examples and make sure everything was working correctly and not having the full scope of everything going on, there's probably some issues with this code, but it should at least get you pointed in a direction or thought process to help clean it up some. As for the performance of the code, that's not my forte, but my general philosophy is to write legible code first and then optimize as needed to meet the business needs