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 "== '''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...")
 
 
(16 intermediate revisions by the same user not shown)
Line 6: Line 6:
== '''Code Climate Issues and Problem Statements''' ==
== '''Code Climate Issues and Problem Statements''' ==


Codeclimate is a command line interface for the Code Climate analysis platform. It could 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 begin with P to Z.  Those issues includes unsafe use of methods, inappropriate syntax, non-optimal code structure and so on. There are totally 33 different types of issues fixed in this project.
Codeclimate is a command line interface for the Code Climate analysis platform [2]. It could 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 begin with P to Z.  Those issues includes unsafe use of methods, inappropriate syntax, non-optimal code structure and so on. There are totally 37 different types of issues fixed in this project.


====Main issues fixed:====


'''Below is a list of all issues we have fixed.'''
*Unprotected mass assignment
 
*Use a guard clause instead of wrapping the code inside a conditional expression
<code>
*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
1. Unprotected mass assignment
*Move redirect_to "/" out of the conditional.
 
*Block has too many lines
2. Use a guard clause instead of wrapping the code inside a conditional expression
*Useless assignment to variable - initheader.
 
*Replace class var @@assignment_id with a class instance var
3. 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
*Avoid comparing a variable with multiple items in a conditional, use Array#include? Instead
 
*Identical blocks of code found in 2 locations. Consider refactoring.  
4. Move redirect_to "/" out of the conditional.
*Favor unless over if for negative conditions.
 
*Operator = should be surrounded by a single space.  
5. Block has too many lines
*Line is too long
 
* Extra empty line detected at method body beginning.  
6. Identical blocks of code found in 2 locations. Consider refactoring.  
*Unnecessary spacing detected.
 
*Parameters should be whitelisted for mass assignment
7. Favor unless over if for negative conditions.
*Avoid using update_attribute because it skips validations.  
 
*end at 44, 4 is not aligned with def at 39, 2.  
8. Operator = should be surrounded by a single space.  
* Unsafe reflection method const_get called with parameter value  
 
*TODO found
9. Line is too long
* Convert if nested inside else to elsif
 
*Similar blocks of code found in 3 locations. Consider refactoring
10. Extra empty line detected at method body beginning.  
*User controlled method execution
 
*Extra blank line detected
11. Unnecessary spacing detected.
*Use empty lines between method definitions.  
 
*Prefer each over for
12. Parameters should be whitelisted for mass assignment
*Prefer Date or Time over DateTime.
 
*Omit parentheses for ternary conditions
13. Avoid using update_attribute because it skips validations.  
*Do not place comments on the same line as the end keyword.  
 
*Useless assignment to variable - controllers. Did you mean controller?
14. end at 44, 4 is not aligned with def at 39, 2.  
* end at 135, 2 is not aligned with class at 1, 0
 
*Put one space between the method name and the first argument.  
15. Unsafe reflection method const_get called with parameter value  
*Space missing after colon.  
 
*Use the new Ruby 1.9 hash syntax
16. TODO found
*show, edit, update, destroy are not explicitly defined on the controller.  
 
*Rename is_user_ta? to user_ta?
17. Convert if nested inside else to elsif
*Avoid comma after the last item of an array
 
*Move redirect_to view_student_teams_path student_id: student.id out of the conditional
18. Similar blocks of code found in 3 locations. Consider refactoring
 
19. User controlled method execution
 
20. Extra blank line detected
 
21. Use empty lines between method definitions.  
 
22. Prefer each over for
 
23. Prefer Date or Time over DateTime.
 
24. Omit parentheses for ternary conditions
 
25. Do not place comments on the same line as the end keyword.  
 
26. Useless assignment to variable - controllers. Did you mean controller?
 
27. end at 135, 2 is not aligned with class at 1, 0
 
28. Put one space between the method name and the first argument.  
 
29. Space missing after colon.  
 
30. Use the new Ruby 1.9 hash syntax
 
31. show, edit, update, destroy are not explicitly defined on the controller.  
 
32. Rename is_user_ta? to user_ta?
 
33. Avoid comma after the last item of an array
</code>


== '''Implementation''' ==
== '''Implementation''' ==


There are many minor issues such as unnecessary spacing, change "for" to "each" for loop, extra blank line detected, omitted parentheses used and so on. We're not going to show all these minor fixing on this section but just show some important issues we fixed.
There are many minor issues such as unnecessary spacing, change "for" to "each" for loop, extra blank line detected, omitted parentheses used and so on. We're not going to show all these minor fixing on this section but just show some important issues we fixed.
* Unprotected Mass Assignment. This issue appears multiple times in different controllers. An example of how we fixed it in questions_controller is shown below:
[[File:Unprotected_mass_assignment.png|center]]


== '''Testing''' ==
== '''Testing''' ==
Line 95: Line 67:
[[File:One_failure_testing.png]]
[[File:One_failure_testing.png]]


== ''Reference'' ==
===Reference===
 
*1:http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2017/E1784_Fix_mass_assignments_reported_by_Brakeman.rb
 
*2: https://github.com/codeclimate/codeclimate/blob/master/README.md
[[http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2017/E1784_Fix_mass_assignments_reported_by_Brakeman.rb]]'
*The Class GitHub Repository: https://github.com/expertiza/expertiza
[[Code Climate General Description]] https://github.com/codeclimate/codeclimate/blob/master/README.md
*The Class Code Climate:https://codeclimate.com/github/expertiza/expertiza

Latest revision as of 22:47, 25 March 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 could 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 begin with P to Z. Those issues includes unsafe use of methods, inappropriate syntax, non-optimal code structure and so on. There are totally 37 different types of issues fixed in this project.

Main issues fixed:

  • 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, change "for" to "each" for loop, extra blank line detected, omitted parentheses used and so on. We're not going to show all these minor fixing on this section but just show some important issues we fixed.

  • Unprotected Mass Assignment. This issue appears multiple times in different controllers. An example of how we fixed it in questions_controller is shown below:

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 controllers.




Reference