CSC/ECE 517 Spring 2019 - Project E1917. Fix Code Climate issues in controllers with names beginning with P through Z: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
(Created page with "Test")
 
No edit summary
 
(54 intermediate revisions by 3 users not shown)
Line 1: Line 1:
Test
== '''About Expertiza''' ==
Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages[1].
 
== '''Code Climate Issues and Problem Statements''' ==
 
Codeclimate is a command line interface for the Code Climate analysis platform [2]. It can detect code smells which violate ruby/rails best practices. The task is to fix certain code climate issues detected by Code Climate analysis platform in controllers with the name beginning with P through Z. These issues includes unsafe use of methods, inappropriate syntax, non-optimal code structure which violates the DRY principle and so on. There are 37 different types of issues fixed in this project.
 
====List of Issues====
 
*Unprotected mass assignment
*Use a guard clause instead of wrapping the code inside a conditional expression
*Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead
*Move redirect_to "/" out of the conditional.
*Block has too many lines
*Useless assignment to variable - initheader.
*Replace class var @@assignment_id with a class instance var
*Avoid comparing a variable with multiple items in a conditional, use Array#include? Instead
*Identical blocks of code found in 2 locations. Consider refactoring.
*Favor unless over if for negative conditions.
*Operator = should be surrounded by a single space.
*Line is too long
*Extra empty line detected at method body beginning.
*Unnecessary spacing detected.
*Parameters should be whitelisted for mass assignment
*Avoid using update_attribute because it skips validations.
*end at 44, 4 is not aligned with def at 39, 2.
*Unsafe reflection method const_get called with parameter value
*TODO found
*Convert if nested inside else to elsif
*Similar blocks of code found in 3 locations. Consider refactoring
*User controlled method execution
*Extra blank line detected
*Use empty lines between method definitions.
*Prefer each over for
*Prefer Date or Time over DateTime.
*Omit parentheses for ternary conditions
*Do not place comments on the same line as the end keyword.
*Useless assignment to variable - controllers. Did you mean controller?
*end at 135, 2 is not aligned with class at 1, 0
*Put one space between the method name and the first argument.
*Space missing after colon.
*Use the new Ruby 1.9 hash syntax
*show, edit, update, destroy are not explicitly defined on the controller.
*Rename is_user_ta? to user_ta?
*Avoid comma after the last item of an array
*Move redirect_to view_student_teams_path student_id: student.id out of the conditional
 
== '''Implementation''' ==
 
There are many minor issues such as unnecessary spacing, extra blank line detected, extra parentheses used and so on. We're not going to show all these minor fixing in this section. The focus would be on some of the important issues listed above that we fixed.
 
===Issue - Unprotected Mass Assignment===
Mass assignment is a feature of Rails which allows an application to create a record from the values of a hash. Protection for mass assignment is on by default. Query parameters must be explicitly whitelisted via permit in order to be used in mass assignment.This issue appears in different controller files. An example of how we fixed it in questions_controller.rb is shown below:
[[File:Unprotected mass assignment 2.png]]
 
===Issue - Use a guard clause instead of wrapping the code inside a conditional expression.===
Used a return/unless combination to return in case the condition is false.
 
[[File:Guard clause.png]]
 
===Issue - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.===
Robocop checks for the use of Time methods without zone.
 
[[File:Time zone and out of condition.png]]
 
===Issue - Move redirect_to "/" out of the conditional.===
Robocop checks for identical lines at the beginning or end of each branch of a conditional statement.
 
[[File:Time_zone_and_out_of_condition.png]]
 
===Issue - Avoid comparing a variable with multiple items in a conditional, use Array#include? Instead ===
Robocop checks against comparing a variable with multiple items, where `Array#include?` could be used instead to avoid code repetition.
 
[[File:Issue_5.png]]
 
===Issue - Favor unless over if for negative conditions.===
Robocop checks for uses of if with a negated condition.
[[File:Issue_10.png]]
 
===Issue - Parameters should be whitelisted for mass assignment.===
[[File:Whitelist_and_update_attributes.png]]
===Issue - Avoid using update_attribute because it skips validations===
 
The method update_attribute will skip validation. Should use update_attributes instead.
 
[[File:Whitelist_and_update_attributes.png]]
 
===Issue - Unsafe reflection method const_get called with parameter value===
[[File:Unsafe const_get.png]]
 
===Issue - Line is too long.===
Code climate will complain when a line is too long.
 
[[File:Line too long.png]]
 
===Issue - Similar blocks of code found in 3 locations. Consider refactoring===
Using the same piece of code violates the DRY principle. We have refactored the code to avoid the repetitive usage.
[[File:Issue_21.png]]
 
===Issue - Rename is_user_ta? to user_ta?===
Renamed is_user_ta? to user_ta? to follow naming conventions enforced by Rubocop.
 
[[File:Issue_35.png]]
 
===Issue - show, edit, update, destroy are not explicitly defined on the controller.===
Show, edit, update and destroy have been explicitly defined to avoid confusion among filters between different controllers.
 
[[File:Issue_32.png]]
 
===Issue - Use the new Ruby 1.9 hash syntax===
This ruby syntax has been deprecated as of the current ruby version(2.3.7) used in expertiza.
 
[[File:Issue_33.png]]
 
===Issue - Put one space between the method name and the first argument.===
Space has been added between method name and the first argument to make the code easily readable.
 
[[File:Issue_31.png]]
 
===Issue - Do not place comments on the same line as the end keyword.===
Adding comments at the last the line of the code is considered a bad coding practice according to rubocop standards.
 
[[File:Issue_27.png]]
 
== '''Testing''' ==
 
The Expertiza project provides 77 rspec tests under expertiza/spec and 8 of them are related to our controllers files.  After modifying those 28 files, we want to make sure these tests could still pass. We passed all the Rspec tests except for "response_controller", however, we were not requirement to fix the issues for this controller.
 
[[File:Testing pass.png]]
 
 
[[File:One_failure_testing.png]]
 
==Team Members==
*Ziwei Wu
*Nilay Kapadia
*Udita Chattopadhyay
 
==Reference==
*http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2017/E1784_Fix_mass_assignments_reported_by_Brakeman.rb
*https://github.com/codeclimate/codeclimate/blob/master/README.md
*The Class GitHub Repository: https://github.com/expertiza/expertiza
*The Class Code Climate:https://codeclimate.com/github/expertiza/expertiza
*Robocop https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style

Latest revision as of 03:12, 2 April 2019

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages[1].

Code Climate Issues and Problem Statements

Codeclimate is a command line interface for the Code Climate analysis platform [2]. It can detect code smells which violate ruby/rails best practices. The task is to fix certain code climate issues detected by Code Climate analysis platform in controllers with the name beginning with P through Z. These issues includes unsafe use of methods, inappropriate syntax, non-optimal code structure which violates the DRY principle and so on. There are 37 different types of issues fixed in this project.

List of Issues

  • Unprotected mass assignment
  • Use a guard clause instead of wrapping the code inside a conditional expression
  • Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead
  • Move redirect_to "/" out of the conditional.
  • Block has too many lines
  • Useless assignment to variable - initheader.
  • Replace class var @@assignment_id with a class instance var
  • Avoid comparing a variable with multiple items in a conditional, use Array#include? Instead
  • Identical blocks of code found in 2 locations. Consider refactoring.
  • Favor unless over if for negative conditions.
  • Operator = should be surrounded by a single space.
  • Line is too long
  • Extra empty line detected at method body beginning.
  • Unnecessary spacing detected.
  • Parameters should be whitelisted for mass assignment
  • Avoid using update_attribute because it skips validations.
  • end at 44, 4 is not aligned with def at 39, 2.
  • Unsafe reflection method const_get called with parameter value
  • TODO found
  • Convert if nested inside else to elsif
  • Similar blocks of code found in 3 locations. Consider refactoring
  • User controlled method execution
  • Extra blank line detected
  • Use empty lines between method definitions.
  • Prefer each over for
  • Prefer Date or Time over DateTime.
  • Omit parentheses for ternary conditions
  • Do not place comments on the same line as the end keyword.
  • Useless assignment to variable - controllers. Did you mean controller?
  • end at 135, 2 is not aligned with class at 1, 0
  • Put one space between the method name and the first argument.
  • Space missing after colon.
  • Use the new Ruby 1.9 hash syntax
  • show, edit, update, destroy are not explicitly defined on the controller.
  • Rename is_user_ta? to user_ta?
  • Avoid comma after the last item of an array
  • Move redirect_to view_student_teams_path student_id: student.id out of the conditional

Implementation

There are many minor issues such as unnecessary spacing, extra blank line detected, extra parentheses used and so on. We're not going to show all these minor fixing in this section. The focus would be on some of the important issues listed above that we fixed.

Issue - Unprotected Mass Assignment

Mass assignment is a feature of Rails which allows an application to create a record from the values of a hash. Protection for mass assignment is on by default. Query parameters must be explicitly whitelisted via permit in order to be used in mass assignment.This issue appears in different controller files. An example of how we fixed it in questions_controller.rb is shown below:

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

Used a return/unless combination to return in case the condition is false.

Issue - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

Robocop checks for the use of Time methods without zone.

Issue - Move redirect_to "/" out of the conditional.

Robocop checks for identical lines at the beginning or end of each branch of a conditional statement.

Issue - Avoid comparing a variable with multiple items in a conditional, use Array#include? Instead

Robocop checks against comparing a variable with multiple items, where `Array#include?` could be used instead to avoid code repetition.

Issue - Favor unless over if for negative conditions.

Robocop checks for uses of if with a negated condition.

Issue - Parameters should be whitelisted for mass assignment.

Issue - Avoid using update_attribute because it skips validations

The method update_attribute will skip validation. Should use update_attributes instead.

Issue - Unsafe reflection method const_get called with parameter value

Issue - Line is too long.

Code climate will complain when a line is too long.

Issue - Similar blocks of code found in 3 locations. Consider refactoring

Using the same piece of code violates the DRY principle. We have refactored the code to avoid the repetitive usage.

Issue - Rename is_user_ta? to user_ta?

Renamed is_user_ta? to user_ta? to follow naming conventions enforced by Rubocop.

Issue - show, edit, update, destroy are not explicitly defined on the controller.

Show, edit, update and destroy have been explicitly defined to avoid confusion among filters between different controllers.

Issue - Use the new Ruby 1.9 hash syntax

This ruby syntax has been deprecated as of the current ruby version(2.3.7) used in expertiza.

Issue - Put one space between the method name and the first argument.

Space has been added between method name and the first argument to make the code easily readable.

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

Adding comments at the last the line of the code is considered a bad coding practice according to rubocop standards.

Testing

The Expertiza project provides 77 rspec tests under expertiza/spec and 8 of them are related to our controllers files. After modifying those 28 files, we want to make sure these tests could still pass. We passed all the Rspec tests except for "response_controller", however, we were not requirement to fix the issues for this controller.


Team Members

  • Ziwei Wu
  • Nilay Kapadia
  • Udita Chattopadhyay

Reference

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2019_-_Project_E1917._Fix_Code_Climate_issues_in_controllers_with_names_beginning_with_P_through_Z&oldid=123100"