CODEWITHSUNDEEP
rubyrefactoring
bigelow42
bigelow42

Reputation: 38

Should I refactor these nested each blocks?

I'm wondering if there's a way to refactor this method to minimize the levels of indentation. After reading clean code Uncle Bob says once a method gets past two levels it becomes harder to understand what's going on.. I don't think this method is overly complex, but I think it could be cleaned up and look more like a story.

here is my method

def sync!(xml_document, language_id, status_id, file)
  survey = sync_survey.(xml_document, status_id)
  sync_survey_label.(xml_document, survey.id, language_id, file)

  xml_document.xpath('//page').each do |xml_section|
    section = sync_section.(xml_section, survey.id)
    sync_section_label.(xml_section, section.id, language_id)

    xml_section.xpath('.//question_group').each do |xml_question_group|
      question_group = sync_question_group.(xml_question_group, section.id)
      sync_question_group_label.(xml_question_group, question_group.id, language_id)

      xml_question_group.xpath('.//question').each do |xml_question|
        question = sync_question.(xml_question, question_group.id)
        sync_question_label.(xml_question, question.id, language_id)

        xml_question.xpath('.//answerchoice').each do |xml_choice|
          choice = sync_choice.(xml_choice, question.id)
          sync_choice_label.(xml_choice, choice.id, language_id)
        end
      end
    end
  end

  survey
end

So, Im traversing an xml and saving to the corresponding methods. each survey has many sections which has many question_groups which has many questions which has many choices.

I know I could extract each loop into a method but then I would still have 4 levels of indentation.

I want to do something like...

def sync!(xml_document, language_id, status_id, file)
  survey = sync_survey.(xml_document, status_id)
  sync_survey_label.(xml_document, survey.id, language_id, file)
  sync_sections.(xml_document)
  sync_section_labels.(xml_document, language_id)
  sync_question_groups.(xml_document)
  sync_question_group_labels.(xml_document, language_id)
  sync_questions.(xml_document)
  sync_question_labels.(xml_document, language_id)
  sync_choices.(xml_document)
  sync_choice_labels.(xml_document, language_id)

  survey
end

Are there any ideas to do something like this or make this code more readable? Is it even necessary to refactor it? Any ideas are welcome.

Here is the full class

class SurveyBuilder
  attr_reader  :xml_parser, :sync_survey, :sync_survey_label, :sync_section, :sync_section_label, :sync_question, :sync_question_label, :sync_question_group, :sync_question_group_label, :sync_choice, :sync_choice_label

  def initialize
    @sync_survey = SurveyBuilder::Sync::Survey.new
    @sync_survey_label = SurveyBuilder::Sync::SurveyLabel.new
    @sync_section = SurveyBuilder::Sync::Section.new
    @sync_section_label = SurveyBuilder::Sync::SectionLabel.new
    @sync_question = SurveyBuilder::Sync::Question.new
    @sync_question_label = SurveyBuilder::Sync::QuestionLabel.new
    @sync_question_group = SurveyBuilder::Sync::QuestionGroup.new
    @sync_question_group_label = SurveyBuilder::Sync::QuestionGroupLabel.new
    @sync_choice = SurveyBuilder::Sync::Choice.new
    @sync_choice_label = SurveyBuilder::Sync::ChoiceLabel.new
    @xml_parser = SurveyBuilder::XMLParser.new
  end

  def call(file, status_id)
    xml_document = xml_parser.(file)
    language_id = Language.find_by(code: xml_document.xpath('//lang_code').children.first.to_s).id

    survey = sync!(xml_document, language_id, status_id, file)

    survey
  end

private
  def sync!(xml_document, language_id, status_id, file)
    survey = sync_survey.(xml_document, status_id)
    sync_survey_label.(xml_document, survey.id, language_id, file)

    xml_document.xpath('//page').each do |xml_section|
      section = sync_section.(xml_section, survey.id)
      sync_section_label.(xml_section, section.id, language_id)

      xml_section.xpath('.//question_group').each do |xml_question_group|
        question_group = sync_question_group.(xml_question_group, section.id)
        sync_question_group_label.(xml_question_group, question_group.id, language_id)

        xml_question_group.xpath('.//question').each do |xml_question|
          question = sync_question.(xml_question, question_group.id)
          sync_question_label.(xml_question, question.id, language_id)

          xml_question.xpath('.//answerchoice').each do |xml_choice|
            choice = sync_choice.(xml_choice, question.id)
            sync_choice_label.(xml_choice, choice.id, language_id)
          end
        end
      end
    end
    survey
  end

end

Upvotes: 0

Views: 149

Answers (1)

Marc Rohloff
Marc Rohloff

Reputation: 1352

My first stab would be something like:

def sync!(xml_document, language_id, status_id, file)
  survey = sync_survey.(xml_document, status_id)
  sync_survey_label.(xml_document, survey.id, language_id, file)

  xml_document.xpath('//page').each do |xml_section|
    sync_section(xml_section, ...)
  end

  survey
end

def sync_section(xml_section, ...)
  ...
  xml_section.xpath('.//question_group').each do |xml_question_group|
    sync_question_group(xml_question_group, ...)
  end
end 

def sync_question_group(xml_question_group, ...)
  ...
  xml_question_group.xpath('.//question').each do |xml_question|
    sync_question(xml_question, ...)
  end
end

def sync_question(xml_question, ...)
  ... 
  xml_question.xpath('.//answerchoice').each do |xml_choice|
    sync_choice(xml_choice, ...)
  end
end

def sync_choice(xml_choice, ...)
  ...
end

I elided a fair bit since I am not sure how you defined the existing sync_question_group which is used in a confusing way with the dot-syntax.

Upvotes: 1

Related Questions