Use of inheritance using two Ruby libraries

Question

Currently I'm working on parsing data from a form service into a pdf form. I created 2 classes one inheriting from the other one. However, I see that the classes I created are growing in lines of code and I'm concern that I am concern that these classes become hard to maintain.

I would like to request feedback from the community regarding OOP principles, style violations and best practices. If there are also security concerns points those out. Here are the classes using inheritance.

require 'open-uri'

class FillablePdfForm
  attr_writer :template_path
  attr_reader :attributes

  def initialize
    fill_out
  end

  def export(file_name, output_file_path: nil)
    output_file_path ||= "#{Rails.root}/tmp/pdfs/#{application_date}_#{which_application}_#{what_status}_#{file_name}.pdf"
    pdftk.fill_form template_path, output_file_path, attributes
    output_file_path
  end

  def form_fields
    pdftk.get_field_names template_path
  end

  def template_path
    pdf_form_application
    @template_path ||=
      "#{Rails.root}/public/pdfs/#{which_application.downcase}_#{what_status.downcase}.pdf"
  end

  protected

  def application_date
    Time.now.strftime('%Y-%m-%d')
  end

  def pdf_form_application
    File.open("#{Rails.root}/public/pdfs/#{which_application.downcase}_#{what_status.downcase}.pdf", "wb") do |file|
      file << URI.parse(url_of_pdf_form).read
    end
  end

  def url_of_pdf_form
    @form_fields.find do |field|
      field['label'] == "#{which_application}_#{what_status}_URL"
    end['default']
  end

  def attributes
    @attributes ||= {}
  end

  def fill(key, value)
    attributes[key.to_s] = value
  end

  def pdftk
    @pdftk ||= PdfForms.new
  end

  def fill_out
    raise 'Must be overridden by child class'
  end
end

Also, I'm passing in the constructor FormStack::Form.new but I was wondering if I should pass it as an argument.

class PdfScrie < FillablePdfForm
  def initialize(user_submission_data, form_fields)
    @user_submission_data = user_submission_data
    @form_fields = form_fields
    @formstack = FormStack::Form.new
    super()
  end

  private

  # PDF Constants
  VALUE_CHECKBOX_ON = 'On'.freeze
  OPTION_SEP = ' | '.freeze
  LABEL_APPLICATION = 'APPLICATION'.freeze
  LABEL_STATUS = 'STATUS'.freeze

  def fill_out
    form_fields.each do |field| # PDF form fields
      unless dictionary[field]
        Rails.logger.warn "#{self.class.name}: Missing \"#{field}\" mapping."
        next
      end

      id = dictionary[field].split(OPTION_SEP)[0]

      @user_submission_data
        .select { |field_data| field_data[FormStack::Form::ATTR_FIELD_ID] == id }
        .each { |field_data| fill_form_with_data(field, field_data) }
    end
  end

  def fill_form_with_data(field, field_data)
    field_number = field_data[FormStack::Form::ATTR_FIELD_ID]
    value = field_data[FormStack::Form::ATTR_FIELD_VALUE]
    field_type = FormStack::Form::field_type(@form_fields, field_number)
    self_method = "fill_#{field_type}".to_sym

    if self.respond_to?(self_method, :include_private)
      send(self_method, field_number, field, value)
    else
      fill(field, value)
    end
  end

  # Field Type Methods

  def fill_address(field_number, field, value)
    address_by_section = FormStack::Form.parse_formstack_nested_attrs(value)
    address_by_section.each do |section, value|
      fill(field, value) if form_field_has_section?(field, section) ||
        FormStack::Form::address_section_aparment?(field, section)
    end
  end

  def fill_phone(field_number, field, value)
    parse_phone_number(value)
    fill(field, @phone_number_sections.shift)
  end

  def fill_name(field_number, field, value)
    full_name = FormStack::Form::parse_name(value)
    fill(field, full_name)
  end

  def fill_checkbox(field_number, field, value)
    if FormStack::Form::field_is_grouped_checkbox(@form_fields, field_number)
      FormStack::Form::parse_checked_options(value).each do |option|
        fill(field, VALUE_CHECKBOX_ON) if checked_option_matches_value(field, option)
      end
    else
      fill(field, value)
    end
  end

  # END Field Type Methods

  # Helpers

  def checked_option_matches_value(field, option)
    dictionary[field].split(OPTION_SEP)[1].include?(option)
  end

  def parse_phone_number(phone_number)
    if phone_number_sections_empty?
      @phone_number_sections = FormStack::Form::parse_phone(phone_number)
    end
  end

  def phone_number_sections_empty?
    @phone_number_sections.nil? || @phone_number_sections.empty?
  end

  def form_field_has_section?(form_field_name, address_section)
    form_field_name.include? address_section.upcase
  end

  def dictionary
    @dictionary ||= JSON.parse(find_dictionary['section_text'])
  end

  def find_dictionary
    @formstack.find_field_by_label("#{which_application}_#{what_status}_DICTIONARY",
                                   @form_fields)
  end

  def which_application
    @formstack.find_value_by_label(LABEL_APPLICATION,
                                   @form_fields,
                                   @user_submission_data)
  end

  def what_status
    @formstack
      .find_value_by_label(LABEL_STATUS, @form_fields, @user_submission_data)
  end
end

Feel free to point out areas of improvement, feedback, best practices and resources.

Answer 1

An OOP suggestion by way of analogy:

Consider a Document and a Cabinet. Should a Document know how to put a copy of itself in a Cabinet? Or should a Cabinet know how to copy a document into itself?

In a small program, either is probably fine. However, it will become unmaintainable as more ways for them to interact are added as the system grows in complexity.

When that happens, there should be an actor at a higher abstraction level, e.g. a "secretary" that makes a copy (perhaps by requesting it via Document#copy) and files the copy into the cabinet (perhaps by requesting it of the Cabinet#file). In their respective isolated context, they don't need to interact or know about each other, so their implementations would not contain references to each other.

If there is only ever "one secretary", just leave the action at the top level abstraction -- the main program. As complexity grows, perhaps a Secretary class can be defined.

However, remember that Secretary's actions are the higher abstraction and Document is a lower abstraction. The dependency directionality is important. A Document shouldn't be imposing a Secretary to act.

Where this applies to your code:

1. export
   • FillablePdfForm is the Document
   • PdfForms is the Cabinet
   • problem: FillablePdfForm#export is the Document putting itself in the Cabinet
2. fill_form_with_data
   • field_data and FormStack::Form are the Documents
   • PdfScrie is the Cabinet
   • problem: PdfScrie#fill_form_with_data is the Cabinet putting the Document in itself

By the way, this concept is the D in SOLID

Another issue is where FillablePdfForm#template_path calls which_application, which is implemented in the subclass Scrie, which the L in SOLID talks about.

The Wikipedia articles are a little thick to get through though, Google around for some alternative explanations of each of the SOLID principles.