CSC/ECE 517 Spring 2019 - Project E1916. Fix Code Climate issues in controllers with names beginning with A through N: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(9 intermediate revisions by 2 users not shown)
Line 5: Line 5:


===Project Description===
===Project Description===
Project E1916. Fix Code Climate issues in controllers with names beginning with A through N
====Project Team Member====
====Project Team Member====
*Shuai Wang (swang28)
*Shuai Wang (swang28)
Line 25: Line 26:
*Potentially dangerous attribute available for mass assignment.
*Potentially dangerous attribute available for mass assignment.


===Files modified in the project===
====Files modified in the project====
In all files in app/controllers/ with names beginning with A through N, except assignment_controller.rb.  
In all files in app/controllers/ with names beginning with A through N, except assignment_controller.rb.  


====Main issue files:====
=====Main issue files:=====


*[MAINTAINABILITY C] app/controllers/assessment360_controller.rb
*[MAINTAINABILITY C] app/controllers/assessment360_controller.rb
Line 58: Line 59:
====Issue: Avoid using update_attribute because it skips validations.====
====Issue: Avoid using update_attribute because it skips validations.====


=====Example=====
*Example
<pre>
<pre>
     if @institution.update_attribute(:name, params[:institution][:name])
     if @institution.update_attribute(:name, params[:institution][:name])
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     if @institution.update_attributes(:name, params[:institution][:name])
     if @institution.update_attributes(:name, params[:institution][:name])
Line 70: Line 71:
====Issue: Issue: Line is too long.====
====Issue: Issue: Line is too long.====


=====Example=====
*Example
<pre>
<pre>
     avg_existing_metareviews = AutomatedMetareview.find_by_sql(["select avg(relevance) as relevance, avg(content_summative) as summative,
     avg_existing_metareviews = AutomatedMetareview.find_by_sql(["select avg(relevance) as relevance, avg(content_summative) as summative,
Line 77: Line 78:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     avg(content_problem) as problem, avg(content_advisory) as advisory,
     avg(content_problem) as problem, avg(content_advisory) as advisory,
Line 87: Line 88:
====Issue: Use normalcase for variable numbers.====
====Issue: Use normalcase for variable numbers.====


=====Example=====
*Example
<pre>
<pre>
     color_1 = 'c53711'
     color_1 = 'c53711'
Line 96: Line 97:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     color1 = 'c53711'
     color1 = 'c53711'
Line 107: Line 108:
====Issue: Keep a blank line before and after protected.====
====Issue: Keep a blank line before and after protected.====


=====Example=====
*Example
<pre>
<pre>
     end
     end
Line 116: Line 117:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     end
     end
Line 128: Line 129:
====Issue:  Surrounding space missing for operator =.====
====Issue:  Surrounding space missing for operator =.====


=====Example=====
*Example
<pre>
<pre>
     temp_avg= @meta_review_info_per_stu[0] * 1.0 / @meta_review_info_per_stu[1]
     temp_avg= @meta_review_info_per_stu[0] * 1.0 / @meta_review_info_per_stu[1]
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     temp_avg = @meta_review_info_per_stu[0] * 1.0 / @meta_review_info_per_stu[1]
     temp_avg = @meta_review_info_per_stu[0] * 1.0 / @meta_review_info_per_stu[1]
Line 140: Line 141:
====Issue: Avoid rescuing without specifying an error class.====
====Issue: Avoid rescuing without specifying an error class.====


=====Example=====
*Example
<pre>
<pre>
     @header_integrated_body.each do |row_hash|
     @header_integrated_body.each do |row_hash|
Line 151: Line 152:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     @header_integrated_body.each do |row_hash|
     @header_integrated_body.each do |row_hash|
Line 164: Line 165:
====Issue: fixed block structure with if.====
====Issue: fixed block structure with if.====


=====Example=====
*Example
<pre>
<pre>
     @has_reviewee = if @model == 'ReviewResponseMap'
     @has_reviewee = if @model == 'ReviewResponseMap'
Line 171: Line 172:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     @has_reviewee = params[:has_reviewee] if @model == 'ReviewResponseMap'
     @has_reviewee = params[:has_reviewee] if @model == 'ReviewResponseMap'
Line 178: Line 179:
====Issue: Prefer Date or Time over DateTime.====
====Issue: Prefer Date or Time over DateTime.====


=====Example=====
*Example
<pre>
<pre>
    due_at = DateTime.parse(params[:due_at])
    due_at = DateTime.parse(params[:due_at])
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     due_at = Date.parse(params[:due_at])
     due_at = Date.parse(params[:due_at])
Line 190: Line 191:
====Issue: Use other_ta_mappings_num.zero? instead of other_ta_mappings_num == 0.====
====Issue: Use other_ta_mappings_num.zero? instead of other_ta_mappings_num == 0.====


=====Example=====
*Example
<pre>
<pre>
     if other_ta_mappings_num == 0
     if other_ta_mappings_num == 0
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     if other_ta_mappings_num.zero?
     if other_ta_mappings_num.zero?
</pre>
====Issue: Use find_by instead of dynamic find_by_login.====
=====Example=====
<pre>
    user = User.find_by_login(params[:login][:name])
</pre>
=====Solution=====
<pre>
    user = User.find_by(params[:login][:name])
</pre>
</pre>


====Issue: Do not place comments on the same line as the end keyword.====
====Issue: Do not place comments on the same line as the end keyword.====


=====Example=====
*Example
<pre>
<pre>
     end # def login
     end # def login
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     end  
     end  
Line 228: Line 217:
====Issue: Use 2 (not 4) spaces for indentation.====
====Issue: Use 2 (not 4) spaces for indentation.====


=====Example=====
*Example
<pre>
<pre>
     when 'index'
     when 'index'
Line 237: Line 226:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     when 'index'
     when 'index'
Line 248: Line 237:
====Issue: use &: instead of block.====
====Issue: use &: instead of block.====


=====Example=====
*Example
<pre>
<pre>
     header.map! { |column_name| column_name.to_sym }
     header.map! { |column_name| column_name.to_sym }
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     header.map! (&:to_sym)
     header.map! (&:to_sym)
Line 260: Line 249:
====Issue: delete redundant parentheses.====
====Issue: delete redundant parentheses.====


=====Example=====
*Example
<pre>
<pre>
     h = Hash.new()
     h = Hash.new()
Line 266: Line 255:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     h = Hash.new
     h = Hash.new
Line 273: Line 262:
====Issue: Don't use parentheses around the condition of an if.====
====Issue: Don't use parentheses around the condition of an if.====


=====Example=====
*Example
<pre>
<pre>
     if (@model == 'AssignmentTeam'|| @model == 'CourseTeam')
     if (@model == 'AssignmentTeam'|| @model == 'CourseTeam')
Line 279: Line 268:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     if @model == 'AssignmentTeam'|| @model == 'CourseTeam'
     if @model == 'AssignmentTeam'|| @model == 'CourseTeam'
Line 286: Line 275:
====Issue: Move @optional_count = 0 out of the conditional.====
====Issue: Move @optional_count = 0 out of the conditional.====


=====Example=====
*Example
<pre>
<pre>
     if (@model == 'SignUpTopic')
     if (@model == 'SignUpTopic')
Line 296: Line 285:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     @optional_count = 0
     @optional_count = 0
Line 308: Line 297:
====Issue: Similar blocks of code found in 2 locations. Consider refactoring. ====
====Issue: Similar blocks of code found in 2 locations. Consider refactoring. ====


=====Example=====
*Example
<pre>
<pre>
     if @teammate_review_info_per_stu[1] > 0
     if @teammate_review_info_per_stu[1] > 0
Line 325: Line 314:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     if @teammate_review_info_per_stu[1] > 0
     if @teammate_review_info_per_stu[1] > 0
Line 342: Line 331:
====Issue: Block has too many lines.====
====Issue: Block has too many lines.====


=====Example=====
*Example
<pre>
<pre>
     populate_hash_for_all_students_all_reviews(assignment,
     populate_hash_for_all_students_all_reviews(assignment,
Line 360: Line 349:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     populate_hash_for_all_students_all_reviews(assignment,cp,teammate_reviews,@teammate_review,@overall_teammate_review_grades,
     populate_hash_for_all_students_all_reviews(assignment,cp,teammate_reviews,@teammate_review,@overall_teammate_review_grades,
Line 370: Line 359:
====Issue: Avoid rescuing the Exception class.====
====Issue: Avoid rescuing the Exception class.====


=====Example=====
*Example
<pre>
<pre>
     rescue Exception => e
     rescue Exception => e
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     rescue StandardError => e
     rescue StandardError => e
Line 382: Line 371:
====Issue: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.====
====Issue: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.====


=====Example=====
*Example
<pre>
<pre>
     unless @assignment_grades[cp.id][assignment_id].nil?
     unless @assignment_grades[cp.id][assignment_id].nil?
Line 389: Line 378:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     @final_grades[cp.id] += @assignment_grades[cp.id][assignment_id] unless @assignment_grades[cp.id][assignment_id].nil?
     @final_grades[cp.id] += @assignment_grades[cp.id][assignment_id] unless @assignment_grades[cp.id][assignment_id].nil?
Line 396: Line 385:
====Issue: Convert if nested inside else to elsif.====
====Issue: Convert if nested inside else to elsif.====


=====Example=====
*Example
<pre>
<pre>
     if params[:set_pressed][:bool] == 'false'
     if params[:set_pressed][:bool] == 'false'
Line 409: Line 398:
</pre>
</pre>


=====Solution=====
*Solution
<pre>
<pre>
     if params[:set_pressed][:bool] == 'false'
     if params[:set_pressed][:bool] == 'false'
Line 421: Line 410:


===Testing===
===Testing===
 
Our task is to fix code smell issues for some of the controller files. In order to prove that our modification did not break the application, we ran all the controller tests to make sure they all pass. We firstly set up the environment through VCL. Then we cloned our repository and simply ran:
<pre>
  $ rspec spec/controllers
</pre>
Then we get the following results which represents that we passes all the controller tests.
[[File:Test_result_1916.png]]
===Reference===
===Reference===
*Our Team GitHub Repository: https://github.com/ece517-p3/expertiza
*Our Team GitHub Repository: https://github.com/ece517-p3/expertiza
*Our Team Code Climate: https://codeclimate.com/github/ece517-p3/expertiza
*Our Team Code Climate: https://codeclimate.com/github/ece517-p3/expertiza
*The Class GitHub Repository: https://github.com/expertiza/expertiza
*The Class GitHub Repository: https://github.com/expertiza/expertiza
*The Class Code Climate:https://codeclimate.com/github/expertiza/expertiza
*The Class Code Climate: https://codeclimate.com/github/expertiza/expertiza
*The testing video: https://www.youtube.com/watch?v=sbAhEYn_JMM&feature=youtu.be

Latest revision as of 22:56, 1 April 2019

Expertiza

Expertiza is a web application developed using Ruby on Rails Framework whose creation and maintenance by students and the faculty of NCSU. The code is available on Github Expertiza on GitHub. In Expertiza, instructor can create new assignments and customize new or existing assignments. The instructor also can create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. All the team member will show under the team member list. A Student can comment on his teammate's performance of the project. Students can also peer review other students' submissions and give tag comment by other's peer review. Students can submit their work by URLs or multitype file submission.

Project Description

Project E1916. Fix Code Climate issues in controllers with names beginning with A through N

Project Team Member

  • Shuai Wang (swang28)
  • Huan Chang (hchang15)
  • Guangyu Yu (gyu22)

Project Task

There is some code smells of expertiza app/controllers detected by the code climate. These violate many of the ruby/rails best practices and needs to be rectified.

Our team is to fix all code smells except:

  • Assignment Branch Condition size for [method name] is too high
  • Perceived complexity for [method name] is too high.
  • Cyclomatic complexity for [method name] is too high.
  • Method [method name] has a Cognitive Complexity of XX (exceeds 5 allowed). Consider refactoring.
  • File [file name] has XXX lines of code (exceeds 250 allowed). Consider refactoring.
  • Class [class name] has XX methods (exceeds 20 allowed). Consider refactoring.
  • Method [method name] has XX lines of code (exceeds 25 allowed). Consider refactoring.
  • Mass assignment is not restricted using attr_accessible.
  • Potentially dangerous attribute available for mass assignment.

Files modified in the project

In all files in app/controllers/ with names beginning with A through N, except assignment_controller.rb.

Main issue files:
  • [MAINTAINABILITY C] app/controllers/assessment360_controller.rb
  • [MAINTAINABILITY B] app/controllers/automated_metareviews_controller.rb
  • [MAINTAINABILITY B] app/controllers/grades_controller.rb
  • [MAINTAINABILITY C] app/controllers/impersonate_controller.rb
  • [MAINTAINABILITY F] app/controllers/import_file_controller.rb
  • [MAINTAINABILITY C] app/controllers/lottery_controller.rb

Typical Issues and Improvements

Issue: Use a guard clause instead of wrapping the code inside a conditional expression.

  • Example
    if assignment.try(:is_selfreview_enabled) and unsubmitted_self_review?(participant.try(:id))
      return false
    else
      return true
    end
  • Solution
    return false if assignment.try(:is_selfreview_enabled) and unsubmitted_self_review?(participant.try(:id))
    true

Issue: Avoid using update_attribute because it skips validations.

  • Example
    if @institution.update_attribute(:name, params[:institution][:name])
  • Solution
    if @institution.update_attributes(:name, params[:institution][:name])

Issue: Issue: Line is too long.

  • Example
    avg_existing_metareviews = AutomatedMetareview.find_by_sql(["select avg(relevance) as relevance, avg(content_summative) as summative,
                                                                avg(content_problem) as problem, avg(content_advisory) as advisory, avg(tone_positive) as positive, avg(tone_negative) as negative,
                                                                avg(tone_neutral) as neutral, avg(quantity) as quantity from automated_metareviews where response_id <> ?", @automated_metareview.response_id])[0]
  • Solution
     avg(content_problem) as problem, avg(content_advisory) as advisory,
                                                                avg(tone_positive) as positive, avg(tone_negative) as negative,
                                                                avg(tone_neutral) as neutral, avg(quantity) as quantity from automated_metareviews where response_id <> ?",
                                                                @automated_metareview.response_id])[0]

Issue: Use normalcase for variable numbers.

  • Example
    color_1 = 'c53711'
    color_2 = '0000ff'
    ...
    bc.data "Your work", current_metareview_data, color_1
    bc.data "Avg. performance on reviews", existing_metareview_data, color_2
  • Solution
    color1 = 'c53711'
    color2 = '0000ff'
    ...
    bc.data "Your work", current_metareview_data, color1
    bc.data "Avg. performance on reviews", existing_metareview_data, color2

Issue: Keep a blank line before and after protected.

  • Example
    end

    protected
    # Use this method to validate the current user in order to avoid allowing users
    # to see unauthorized data.
  • Solution
    end

    protected

    # Use this method to validate the current user in order to avoid allowing users
    # to see unauthorized data.

Issue: Surrounding space missing for operator =.

  • Example
    temp_avg= @meta_review_info_per_stu[0] * 1.0 / @meta_review_info_per_stu[1]
  • Solution
    temp_avg = @meta_review_info_per_stu[0] * 1.0 / @meta_review_info_per_stu[1]

Issue: Avoid rescuing without specifying an error class.

  • Example
     @header_integrated_body.each do |row_hash|
          ReviewResponseMap.import(row_hash, session, params[:id])
        end
      rescue
        errors << $ERROR_INFO
      end
    elsif params[:model] == "MetareviewResponseMap"
  • Solution
     @header_integrated_body.each do |row_hash|
          ReviewResponseMap.import(row_hash, session, params[:id])
        end
      rescue StandardError
        errors << $ERROR_INFO
      end
    elsif params[:model] == "MetareviewResponseMap"

Issue: fixed block structure with if.

  • Example
    @has_reviewee = if @model == 'ReviewResponseMap'
                      params[:has_reviewee]
                    end
  • Solution
    @has_reviewee = params[:has_reviewee] if @model == 'ReviewResponseMap'

Issue: Prefer Date or Time over DateTime.

  • Example
    due_at = DateTime.parse(params[:due_at])
  • Solution
    due_at = Date.parse(params[:due_at])

Issue: Use other_ta_mappings_num.zero? instead of other_ta_mappings_num == 0.

  • Example
    if other_ta_mappings_num == 0
  • Solution
    if other_ta_mappings_num.zero?

Issue: Do not place comments on the same line as the end keyword.

  • Example
    end # def login
  • Solution
    end 

    # def login

Issue: Use 2 (not 4) spaces for indentation.

  • Example
    when 'index'
        ['Instructor',
         'Teaching Assistant',
         'Student',
         'Administrator'].include? current_role_name
  • Solution
    when 'index'
      ['Instructor',
       'Teaching Assistant',
       'Student',
       'Administrator'].include? current_role_name

Issue: use &: instead of block.

  • Example
    header.map! { |column_name| column_name.to_sym }
  • Solution
    header.map! (&:to_sym)

Issue: delete redundant parentheses.

  • Example
    h = Hash.new()

  • Solution
    h = Hash.new

Issue: Don't use parentheses around the condition of an if.

  • Example
    if (@model == 'AssignmentTeam'|| @model == 'CourseTeam')

  • Solution
    if @model == 'AssignmentTeam'|| @model == 'CourseTeam'

Issue: Move @optional_count = 0 out of the conditional.

  • Example
     if (@model == 'SignUpTopic')
       @optional_count = 0
       if (params[:category] == 'true')
         @optional_count += 1
       end
     end
  • Solution
    @optional_count = 0
    if @model == 'SignUpTopic'
      if params[:category] == 'true'
        @optional_count += 1
      end
    end

Issue: Similar blocks of code found in 2 locations. Consider refactoring.

  • Example
    if @teammate_review_info_per_stu[1] > 0
      calculate_avg_grade(@teammate_review_info_per_stu, @teammate_review, cp.id)
    end
    if @meta_review_info_per_stu[1] > 0
      calculate_avg_grade(@meta_review_info_per_stu, @meta_review, cp.id)
    end

    ...

    def calculate_avg_grade(review_info_per_stu, review, cp.id)
      temp_avg_grade = review_info_per_stu[0] * 1.0 / review_info_per_stu[1]
      review[cp.id][:avg_grade_for_assgt] = temp_avg_grade.round.to_s + '%'
    end
  • Solution
    if @teammate_review_info_per_stu[1] > 0
      s = @teammate_review_info_per_stu[0] * 1.0
      temp_avg_grade = s / @teammate_review_info_per_stu[1]
      t = temp_avg_grade.round.to_s
      @teammate_review[cp.id][:avg_grade_for_assgt] = t + '%'
    end
    if @meta_review_info_per_stu[1] > 0
      s1 = @meta_review_info_per_stu[1]
      temp_avg= @meta_review_info_per_stu[0] * 1.0 / s1
      @meta_review[cp.id][:avg_grade_for_assgt] = temp_avg.round.to_s + '%'
    end

Issue: Block has too many lines.

  • Example
    populate_hash_for_all_students_all_reviews(assignment,
                                                   cp,
                                                   teammate_reviews,
                                                   @teammate_review,
                                                   @overall_teammate_review_grades,
                                                   @overall_teammate_review_count,
                                                   @teammate_review_info_per_stu)
    populate_hash_for_all_students_all_reviews(assignment,
                                                   cp,
                                                   meta_reviews,
                                                   @meta_review,
                                                   @overall_meta_review_grades,
                                                   @overall_meta_review_count,
                                                   @meta_review_info_per_stu)
  • Solution
    populate_hash_for_all_students_all_reviews(assignment,cp,teammate_reviews,@teammate_review,@overall_teammate_review_grades,
                                               @overall_teammate_review_count,@teammate_review_info_per_stu)
    populate_hash_for_all_students_all_reviews(assignment,cp,meta_reviews,@meta_review,@overall_meta_review_grades,
                                               @overall_meta_review_count,@meta_review_info_per_stu)

Issue: Avoid rescuing the Exception class.

  • Example
    rescue Exception => e
  • Solution
    rescue StandardError => e

Issue: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

  • Example
     unless @assignment_grades[cp.id][assignment_id].nil?
       @final_grades[cp.id] += @assignment_grades[cp.id][assignment_id]
     end
  • Solution
     @final_grades[cp.id] += @assignment_grades[cp.id][assignment_id] unless @assignment_grades[cp.id][assignment_id].nil?

Issue: Convert if nested inside else to elsif.

  • Example
    if params[:set_pressed][:bool] == 'false'
      flash[:error] = "There has been some submissions for the rounds of reviews that you're trying to reduce. You can only increase the round of review."
    else
      if @assignment_form.update_attributes(assignment_form_params, current_user)
        flash[:note] = 'The assignment was successfully saved....'
      else
        flash[:error] = "Failed to save the assignment: #{@assignment_form.errors.get(:message)}"
      end
    end
  • Solution
    if params[:set_pressed][:bool] == 'false'
      flash[:error] = "There has been some submissions for the rounds of reviews that you're trying to reduce. You can only increase the round of review."
    elsif @assignment_form.update_attributes(assignment_form_params, current_user)
      flash[:note] = 'The assignment was successfully saved....'
    else
      flash[:error] = "Failed to save the assignment: #{@assignment_form.errors.get(:message)}"
    end

Testing

Our task is to fix code smell issues for some of the controller files. In order to prove that our modification did not break the application, we ran all the controller tests to make sure they all pass. We firstly set up the environment through VCL. Then we cloned our repository and simply ran:

  $ rspec spec/controllers

Then we get the following results which represents that we passes all the controller tests.

Reference