CSC/ECE 517 Fall 2014/oss E1502 wwj: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(19 intermediate revisions by the same user not shown)
Line 1: Line 1:
==E1502: Questionnaire Controller Refactoring==
==E1502: Questionnaire Controller Refactoring==
<span style="color:red">red writing</span>
In this project, we have done some refactoring on the questionnaires_controller. We moved some methods to where we thought suited them, and changed the format of language into ruby style, deleted debugging statement. Details are displayed in the following sections.
 
==Introduction to Expertiza==
==Introduction to Expertiza==
The Expertiza project is system for using peer review to create reusable learning objects. Students do different assignments; then peer review selects the best work in each category, and assembles it to create a single unit.<ref>[http://expertiza.ncsu.edu/ Expertiza]</ref>


==Project Description==
==Project Description==
Line 30: Line 32:
| copy
| copy
| Extracted the content of this method as copy_questionnaires method and put it in questionnaire.rb  
| Extracted the content of this method as copy_questionnaires method and put it in questionnaire.rb  
| The content of this method is about operations on the database (coping a questionnaire), it is better to put it in the model
| The content of this method is about operations on the database (coping a questionnaire), it is better to be put in the model
|-
|-
| valid_quiz
| valid_quiz
Line 49: Line 51:
|-
|-
|}
|}
===Format Refactoring===
===Format Refactoring===
====Case 1: Loop====
====Case 1: Loop Condition====
Change all from "for question in @questionnaire.questions" to "@questionnaire.questions.each do |question|"<br>
Change all the looping conditions into one format<br>
<b>Before</b>
<b>Before</b>
<pre>
<pre>
  # Remove a given questionnaire
  # Remove a given questionnaire
   def delete
   def delete
    @questionnaire = Questionnaire.find(params[:id])
  .
 
  .
    if @questionnaire
  .
      begin
  for question in @questionnaire.questions
        name = @questionnaire.name
  .
 
  .
        for question in @questionnaire.questions
  .
          current_q_type = QuestionType.find_by_question_id(question.id)
          unless current_q_type.nil?
            current_q_type.delete
          end
        end
        @questionnaire.assignments.each{
          | assignment |
          raise "The assignment #{assignment.name} uses this questionnaire. Do you want to <A href='../assignment/delete/#{assignment.id}'>delete</A> the assignment?"
        }
        @questionnaire.destroy
        undo_link("Questionnaire \"#{name}\" has been deleted successfully. ")
      rescue
        flash[:error] = $!
      end
    end
 
    redirect_to :action => 'list', :controller => 'tree_display'
   end
   end
</pre>
</pre>
<b>After</b>
<b>After</b>
<pre>
<pre>
# Remove a given questionnaire
# Remove a given questionnaire
   def delete
   def delete
    @questionnaire = Questionnaire.find(params[:id])
  .
  .
  .
  @questionnaire.assignments.each
  .
  .
  .
  end
</pre>


    if @questionnaire
====Case 2: If Condition====
      begin
Change all the if conditions into ruby format<br>
        name = @questionnaire.name
<b>Before</b>
<pre>
.
.
if @successful_create == true
.
.
unless this_q.nil?
.
.
</pre>
<b>After</b>
<pre>
.
.
if @successful_create
.
.
if this_q
.
.
</pre>


        @questionnaire.questions.each do |question|
====Case 3: Name====
          current_q_type = QuestionType.find_by_question_id(question.id)
Change all the name from "JAVA name" to "Ruby name"<br>
          if current_q_type
            current_q_type.delete
          end
        end
        @questionnaire.assignments.each{
          | assignment |
          raise "The assignment #{assignment.name} uses this questionnaire. Do you want to <A href='../assignment/delete/#{assignment.id}'>delete</A> the assignment?"
        }
        @questionnaire.destroy
        undo_link("Questionnaire \"#{name}\" has been deleted successfully. ")
      rescue
        flash[:error] = $!
      end
    end
 
    redirect_to :action => 'list', :controller => 'tree_display'
  end
</pre>
====Case 2: Name====
Change all the name from "JAVA name" to "Ruby name"
<b>Before</b>
<b>Before</b>
<pre>
<pre>
Line 140: Line 135:
  end
  end
</pre>
</pre>
===Refactoring Example===
<p id="example">In models/questionnaires.rb, we add copy_questionnaires method, as shown below:</p>
<pre>
def self.copy_questionnaires(id,user)
    @orig_questionnaire = Questionnaire.find(id)
    questions = Question.where(questionnaire_id: id)
    @questionnaire = @orig_questionnaire.clone
    @questionnaire.instructor_id = user.instructor_id  ## Why was TA-specific code removed here?  See Project E713.
    @questionnaire.name = 'Copy of ' + @orig_questionnaire.name
    if user.role.name != "Teaching Assistant"
      @questionnaire.instructor_id = user.id
    else # for TA we need to get his instructor id and by default add it to his course for which he is the TA
      @questionnaire.instructor_id = Ta.get_my_instructor(user.id)
    end
    @questionnaire.name = 'Copy of '+@orig_questionnaire.name
      @questionnaire.created_at = Time.now
      @questionnaire.save!
      questions.each{ | question |
        new_question = question.clone
        new_question.questionnaire_id = @questionnaire.id
        new_question.save
        advice = QuestionAdvice.find_by_question_id(question.id)
        if advice
          new_advice = advice.clone
          new_advice.question_id = newquestion.id
          new_advice.save
        end
        if (@questionnaire.section == "Custom")
          old_question_type = QuestionType.find_by_question_id(question.id)
          if old_question_type
            new_question_type = old_question_type.clone
            new_question_type.question_id = newquestion.id
            new_question_type.save
          end
        end
      }
      pFolder = TreeFolder.find_by_name(@questionnaire.display_type)
      parent = FolderNode.find_by_node_object_id(pFolder.id)
      unless QuestionnaireNode.where(parent_id: parent.id, node_object_id: @questionnaire.id)
        QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
      end
  end
</pre>
==References==
<references/>

Latest revision as of 23:44, 21 March 2015

E1502: Questionnaire Controller Refactoring

In this project, we have done some refactoring on the questionnaires_controller. We moved some methods to where we thought suited them, and changed the format of language into ruby style, deleted debugging statement. Details are displayed in the following sections.

Introduction to Expertiza

The Expertiza project is system for using peer review to create reusable learning objects. Students do different assignments; then peer review selects the best work in each category, and assembles it to create a single unit.<ref>Expertiza</ref>

Project Description

What it does:

Used on the admin side of Expertiza for creating/ editing questionnaires (rubrics, surveys and quizzes). It helps in add/removing questions, options, etc for a questionnaire.

  • Very big controller that handles a lot more than the name suggests. Functionalities need to be moved to appropriate controllers.
  • Quiz methods are should be treated the same as any other type of questionnaire; differences between quiz questionnaires and other questionnaires should be implemented in the model class, quiz_questionnaire
  • Turn the questionnaire into a “form object.” The ..._questions methods: save_new_questions, delete_questions, save_questions should be in a separate class.

    Other classes involved:

  • questionnaire.rb
  • quiz_questionnaire.rb
  • questions_controller.rb

    What needs to be done:

  • Move quiz related functions to quiz_questionnaire.rb.
  • copy, update_quiz, valid_quiz methods, clone_questionnaire_details is too long.
  • Debug output (print statements) should be removed.
  • Understand the functions in the controller and comment them. Ensure that the code is understandable to the next programmer who works on it.

    What We Have Done

    Method Refactoring

    Method Name Changes Made Reason For Change
    copy Extracted the content of this method as copy_questionnaires method and put it in questionnaire.rb The content of this method is about operations on the database (coping a questionnaire), it is better to be put in the model
    valid_quiz Moved this method to quiz_questionnaire.rb This method is about validation of the quiz, it shouldn't appear in the controller
    export Moved this method to questionnaire.rb This method exports the questionnaires as csv file, it should't appear in the controller
    import Moved this method to questionnaire.rb This method imports the questionnaires from csv file, it should't appear in the controller
    clone_questionnaire_details Deleted this method due to the duplication Substituted by copy_questionnaires method in questionnaire.rb

    Format Refactoring

    Case 1: Loop Condition

    Change all the looping conditions into one format
    Before

     # Remove a given questionnaire
      def delete
      .
      .
      .
      for question in @questionnaire.questions
      .
      .
      .
      end
    

    After

     # Remove a given questionnaire
      def delete
      .
      .
      .
      @questionnaire.assignments.each
      .
      .
      .
      end
    

    Case 2: If Condition

    Change all the if conditions into ruby format
    Before

     .
     .
     if @successful_create == true
     .
     . 
     unless this_q.nil?
     .
     .
    

    After

     .
     .
     if @successful_create
     .
     .
     if this_q
     .
     .
    

    Case 3: Name

    Change all the name from "JAVA name" to "Ruby name"
    Before

     #save an updated quiz questionnaire to the database
      def update_quiz
      .
      .
      .
      questionnum=1
      .
      .
      .
     end
    

    After

     #save an updated quiz questionnaire to the database
      def update_quiz
      .
      .
      .
      question_num=1
      .
      .
      .
     end
    

    Refactoring Example

    In models/questionnaires.rb, we add copy_questionnaires method, as shown below:

    def self.copy_questionnaires(id,user)
        @orig_questionnaire = Questionnaire.find(id)
        questions = Question.where(questionnaire_id: id)
        @questionnaire = @orig_questionnaire.clone
        @questionnaire.instructor_id = user.instructor_id  ## Why was TA-specific code removed here?  See Project E713.
        @questionnaire.name = 'Copy of ' + @orig_questionnaire.name
    
        if user.role.name != "Teaching Assistant"
          @questionnaire.instructor_id = user.id
        else # for TA we need to get his instructor id and by default add it to his course for which he is the TA
          @questionnaire.instructor_id = Ta.get_my_instructor(user.id)
        end
        @questionnaire.name = 'Copy of '+@orig_questionnaire.name
    
    
          @questionnaire.created_at = Time.now
          @questionnaire.save!
    
          questions.each{ | question |
    
            new_question = question.clone
            new_question.questionnaire_id = @questionnaire.id
            new_question.save
    
            advice = QuestionAdvice.find_by_question_id(question.id)
            if advice
              new_advice = advice.clone
              new_advice.question_id = newquestion.id
              new_advice.save
            end
    
            if (@questionnaire.section == "Custom")
              old_question_type = QuestionType.find_by_question_id(question.id)
              if old_question_type
                new_question_type = old_question_type.clone
                new_question_type.question_id = newquestion.id
                new_question_type.save
              end
            end
          }
          pFolder = TreeFolder.find_by_name(@questionnaire.display_type)
          parent = FolderNode.find_by_node_object_id(pFolder.id)
          unless QuestionnaireNode.where(parent_id: parent.id, node_object_id: @questionnaire.id)
            QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
          end
      end
    

    References

    <references/>