# Comparing verbosity of algorithmic Ruby class

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


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

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

@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

• I like your take on it! Upvoted. – Xenyal Mar 9 '20 at 15:16
• For more context, @pdf_field_uuids_to_remove and @page_to_fd_components_hash are parameters that I send to another application actually - so maybe casting the components into classes might be overkill in this circumstance since I'd have to serialize them back during my request. – Xenyal Mar 9 '20 at 15:17