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.