2017-02-23 61 views
0

我想知道是否有一种方法来重构此方法以最大限度地缩减缩进级别。读完干净的代码后,鲍勃叔叔说,一旦方法超过两个级别,就很难理解正在发生的事情。我不认为这种方法过于复杂,但我认为它可以被清理,看起来更像一个故事。我应该重构这些嵌套的每个块吗?

这里是我的方法

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 

所以,林遍历XML并保存到相应的方法。每个调查都有很多部分,其中有很多问题组有很多选择的问题。

我知道我可以提取每个循环到一个方法,但我仍然会有4个级别的缩进。

我想要做这样的事情......

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 

是否有任何想法,做这样的事情还是让这段代码更易读?是否有必要重构它?任何想法都欢迎。

这里是满级

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 
+0

'sync_survey。(xml_document,status_id)'这是什么语法? –

+0

这是调用方法的简写。所以sync_survey是一个具有调用方法的对象,它可以查找并初始化调查并保存它。它可以写成'sync_survey.call(xml_document,status_id' @EricDuminil – bigelow42

+0

我认为你可能会更好的重构为'sync_survey.sync!(xml_document,status_id)'或类似的东西来清晰起见。它也不是很清楚如何创建这些对象以及它们如何适合您的示例 –

回答

1

我的第一个尝试会是这样的:

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 

我省略的公平一点,因为我不知道你是如何定义的现有sync_question_group所使用以一种令人困惑的方式使用点语法。

+0

这也是我的第一个念头我唯一不喜欢的是每种方法都做了不止一件事sync_section会保存每个部分然后调用sync_question_group会叫同步问题...等等也许我想做的事情是不可能的在这个场景中我编辑了我的原始问题来显示完整的类@MarcRohloff – bigelow42

+0

你可能会把它分解得更远我想你最初想要的东西像:'sync_sections。(xml_document); ...; sync_question_groups(xml_document)' 但我看到的问题是,每个曲estion组需要访问它的部分。您必须将该最后一位更改为: 'sync_question_groups。(xml_document,sync_sections)' –