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

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 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: 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: 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: 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

Reference

• Our Team GitHub Repository: https://github.com/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 Code Climate:https://codeclimate.com/github/expertiza/expertiza