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

2019-03-31T20:34:10Z

Hchang15:

__TOC__

===Expertiza===
[http://expertiza.ncsu.edu/ 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 [https://github.com/expertiza/expertiza 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
<pre>
if assignment.try(:is_selfreview_enabled) and unsubmitted_self_review?(participant.try(:id))
return false
else
return true
end
</pre>

*Solution
<pre>
return false if assignment.try(:is_selfreview_enabled) and unsubmitted_self_review?(participant.try(:id))
true
</pre>

====Issue: Avoid using update_attribute because it skips validations.====

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

*Solution
<pre>
if @institution.update_attributes(:name, params[:institution][:name])
</pre>

====Issue: Issue: Line is too long.====

*Example
<pre>
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]
</pre>

*Solution
<pre>
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]
</pre>

====Issue: Use normalcase for variable numbers.====

*Example
<pre>
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
</pre>

*Solution
<pre>
color1 = 'c53711'
color2 = '0000ff'
...
bc.data "Your work", current_metareview_data, color1
bc.data "Avg. performance on reviews", existing_metareview_data, color2
</pre>

====Issue: Keep a blank line before and after protected.====

*Example
<pre>
end

protected
# Use this method to validate the current user in order to avoid allowing users
# to see unauthorized data.
</pre>

*Solution
<pre>
end

protected

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

====Issue: Surrounding space missing for operator =.====

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

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

====Issue: Avoid rescuing without specifying an error class.====

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

*Solution
<pre>
@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"
</pre>

====Issue: fixed block structure with if.====

*Example
<pre>
@has_reviewee = if @model == 'ReviewResponseMap'
params[:has_reviewee]
end
</pre>

*Solution
<pre>
@has_reviewee = params[:has_reviewee] if @model == 'ReviewResponseMap'
</pre>

====Issue: Prefer Date or Time over DateTime.====

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

*Solution
<pre>
due_at = Date.parse(params[:due_at])
</pre>

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

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

*Solution
<pre>
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>

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

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

*Solution
<pre>
end

# def login
</pre>

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

*Example
<pre>
when 'index'
['Instructor',
'Teaching Assistant',
'Student',
'Administrator'].include? current_role_name
</pre>

*Solution
<pre>
when 'index'
['Instructor',
'Teaching Assistant',
'Student',
'Administrator'].include? current_role_name
</pre>

====Issue: use &: instead of block.====

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

*Solution
<pre>
header.map! (&:to_sym)
</pre>

====Issue: delete redundant parentheses.====

*Example
<pre>
h = Hash.new()

</pre>

*Solution
<pre>
h = Hash.new
</pre>

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

*Example
<pre>
if (@model == 'AssignmentTeam'|| @model == 'CourseTeam')

</pre>

*Solution
<pre>
if @model == 'AssignmentTeam'|| @model == 'CourseTeam'
</pre>

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

*Example
<pre>
if (@model == 'SignUpTopic')
@optional_count = 0
if (params[:category] == 'true')
@optional_count += 1
end
end
</pre>

*Solution
<pre>
@optional_count = 0
if @model == 'SignUpTopic'
if params[:category] == 'true'
@optional_count += 1
end
end
</pre>

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

*Example
<pre>
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
</pre>

*Solution
<pre>
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
</pre>

====Issue: Block has too many lines.====

*Example
<pre>
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)
</pre>

*Solution
<pre>
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)
</pre>

====Issue: Avoid rescuing the Exception class.====

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

*Solution
<pre>
rescue StandardError => e
</pre>

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

*Example
<pre>
unless @assignment_grades[cp.id][assignment_id].nil?
@final_grades[cp.id] += @assignment_grades[cp.id][assignment_id]
end
</pre>

*Solution
<pre>
@final_grades[cp.id] += @assignment_grades[cp.id][assignment_id] unless @assignment_grades[cp.id][assignment_id].nil?
</pre>

====Issue: Convert if nested inside else to elsif.====

*Example
<pre>
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
</pre>

*Solution
<pre>
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
</pre>

===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===
*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

File:Test result 1916.png

2019-03-31T20:28:41Z

Hchang15: test result for task 1916

test result for task 1916

Expertiza_Wiki - User contributions [en]

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

File:Test result 1916.png