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.