CSC/ECE 517 Spring 2019 E1910 Refactor assignments controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
 
(33 intermediate revisions by 3 users not shown)
Line 23: Line 23:
2. Assignment_helper
2. Assignment_helper


The updated methods in AssignmentController:
The updated method names in AssignmentController:
   
   
   - handle_assignment_directory_path_nonexist_case_and_answer_tagging  
   - handle_assignment_directory_path_nonexist_case_and_answer_tagging  
Line 29: Line 29:
   - assignment_form_key_nonexist_case_handler  
   - assignment_form_key_nonexist_case_handler  
     -- assignment_submission_handler
     -- assignment_submission_handler
   - Handle_current_user_timezonepref_nil
   - handle_current_user_timezonepref_nil
     -- timezone_handler
     -- timezone_handler
   - update_nil_dd_deadline_name  
   - update_nil_dd_deadline_name  
Line 42: Line 42:
     -- update_feedback_attributes
  
     -- update_feedback_attributes
  


We have left a few comments in the controller that say 'used to be...' so that it can be seen where the changes have been made and these will be removed before the final submission.  
The thought process behind changing the method names was to make the names shorter and reduce redundancy. There were a few rules that we learned from our mentor; words like ''nil'' and ''nonexist'' are not allowed in method names. So for '''handle_current_user_timezonepref_nil''' we first removed the ''nil''. Then after looking at what the method does we determined that '''timezone_handler''' would be appropriate and tells the user enough at a moments glance. More information is available about the method in the form of a block comment.


The assignment_helper file was added to. Some of the methods in the controller were misplaced and according to the DRY principle these methods are better suited to  be in the helper method. The methods that were moved from the controller to the helper were:
Another example is '''assignment_form_assignment_staggered_deadline?''', the ''assignment_form'' bit is redundant, since it is the assignments controller the beginning part does not provide any new information and '''assignment_staggered_deadline?''' is sufficient.
 
A different kind of naming is '''update_nil_dd_deadline_name''', we removed the ''nil'' but the ''dd'' doesn't make sense. No one would know what that stands for just by reading it. This is why this method was changed to '''update_due_date_deadline_name'''. It is not any shorter but much more useful at a glance.
 
 
The '''assignment_helper''' file was added to. Some of the methods in the controller were misplaced and according to the DRY principle these methods are better suited to  be in the helper method. The methods that were moved from the controller to the helper were:
   - assignment_submission_handler -> updated method name
   - assignment_submission_handler -> updated method name
   - check_due_date_nameurl(dd) -> updated method name
   - check_due_date_nameurl(dd) -> updated method name
We were asked to refactor the '''action_allowed?''' method but after discussing some reafactoring methods with our mentor it was decided that refactoring this method would not promote the DRY principle and the methods created as a result would only be for that one purpose and therefore unnecessary.


== Code Improvements ==
== Code Improvements ==
Line 52: Line 60:
The DRY principle states, "Every piece of knowledge or logic must have a single, unambiguous representation within a system."[4] This project was about furthering this principle and keeping this controller concise and maintainable.  
The DRY principle states, "Every piece of knowledge or logic must have a single, unambiguous representation within a system."[4] This project was about furthering this principle and keeping this controller concise and maintainable.  


This is a controller that helps instructors, TA's, administrators, and super-administrators create/update/edit/show/delete current and past assignments. We updated the aforementioned method names so that the naming convention follows the DRY principle. Many of the method names were far too long or repetitive or unhelpful in understanding what the method actually completed. In accordance with the principle we updated the method names to be simple while also being useful in letting the programmer know what the method does.  
 
This is a controller that helps instructors, TA's, administrators, and super-administrators create/update/edit/show/delete current and past assignments. We updated the aforementioned method names so that the naming convention follows the DRY principle. Many of the method names were far too long, repetitive or unhelpful in understanding what the method actually does. In accordance with the principle we updated the method names to be simple while also being useful in letting the programmer know what the method does.  
 


Many of these methods did not have any comments to help the programmer understand the logic driving the method. To further help the programmer in understanding what the code does we added block comments at the beginning of each method that we updated.  
Many of these methods did not have any comments to help the programmer understand the logic driving the method. To further help the programmer in understanding what the code does we added block comments at the beginning of each method that we updated.  
    
    
We cleaned up the controller by moving some methods from the controller into the associated helper file (assignment_helper.rb). By doing this we made the controller-less messy and easier to read while maintaining and furthering the DRY principle. The code is now easily maintainable and all "helper" methods are in one place rather than separated across many files.  
We cleaned up the controller by moving some methods from the controller into the associated helper file ('''assignment_helper.rb'''). By doing this we made the controller less messy and easier to read while maintaining and furthering the DRY principle. The code is now easily maintainable and all "helper" methods are in one place rather than separated across many files.
 
 
The delegation pattern states, "In delegation, an object handles a request by delegating to a second object (the delegate). The delegate is a helper object, but with the original context." [5]
 
As something extra, the '''create''' method was refactored so that the branch condition would be lower and there would be no repetition. We decided to use the delegation pattern because it serves the purpose of cleaning up existing methods while maintaining functionality. As per this pattern we took a request from the '''create''' method and put it in a helper method called '''update_assignment_form'''. This method handles updating the id of the ''assignment_questionnaire'' and the ''due_date'' and updating the form accordingly. The '''create''' method delegates this task to the '''update_assignment_form'''.  


The '''create''' method was refactored so that the branch condition would be lower. As per the delegation design pattern we took a part of the functionality from the '''create''' method and put it in its own method called '''update_assignment_form'''. This method handles updating the id of the assignment_questionnaire and the due_date and updating the form accordingly. As a result of this change two code climate issues have been resolved which are stated below.
The '''update_assignment_form''' delegated a tasks to the helper method, '''array_traverser'''. This method does the job of going through the array and updates id fields to be strings instead of an integer.  


As a result of these changes two code climate issues have been resolved which are stated below in the Code Climate section. Below are images of the way the '''create''' method used to look and how it looks now.


Old '''create''' method:
Old '''create''' method:
Line 71: Line 89:




'''update_assignment_form''' method:
===Code Climate===
 
Code Climate checks to make sure that the code that has been written follows certain rules so that everything is legible and concise. During our refactoring we have fixed the following code climate issues.
 
- The '''create''' method's Cognitive Complexity was too high. That has been resolved by the delegation we have described above. <br/>
- The '''create''' method was too long with 33 lines of code. That has been resolved by the delegation we have described above. <br/>
- The Branch Condition size for '''assignment_form_key_nonexist_case_handler''', which is now '''assignment_submission_handler''', was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code. <br/>
- The Branch Condition size for '''update_feedback_assignment_form_attributes''', which is now '''update_feedback_attributes
''', was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code.<br/>
 


[[File:new_method_1.png]]
We have three current code climate issues but after discussing with our mentor we have been told to ignore those.


The '''update_assignment_form''' method had redundancy of code and so we created two different functions for that part and created a generalized function to run the logic based on the option that is passed to the function. This way we overcame the redundancy part in the function by modularizing the logic.
== Test Plan ==


===Code Climate===
The code coverage for the controller was 49% when we got the project. The code coverage when we finished was 53.32%.
 
There is a video on Expertiza titled "Rspec_test.mp4" which shows the updated tests running successfully.


Code Climate checks to make sure that the code that has been written follows certain rules so that everything is legible and concise. During our refactoring we have fixed the following code climate issues.
=== RSpec Tests ===


- The '''create''' method's Cognitive Complexity was too high. That has been resolved by the delegation we have described above.  
There were already quite a few test cases for the AssignmentsController ('''assignments_controller_spec.rb'''). The '''create''' block tested just about everything. The two methods we added are private and since we branched out some functionality this needed to be a bit more explicitly tested. There is no precedence for testing private methods and it was frowned upon as per the video ''The Magic Tricks of Testing''. We tested the public method, '''create''', that calls the private method, '''update_assignment_form''' and '''array_traverser'''. We added a test with the context ''when assignment_form is saved successfully but fails to update''.  
- The '''create''' method was too long with 33 lines of code. That has been resolved by the delegation we have described above.  
- The Branch Condition size for '''assignment_form_key_nonexist_case_handler''', which is now '''assignment_submission_handler''', was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code.
- The Branch Condition size for '''update_feedback_assignment_form_attributes''', which is now '''update_feedback_attributes
''', was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code.


== Testing Details==
For the other untested public methods, we found no usage directly in the controller, therefore, we did not test them. These methods are performing the tasks of assigning values to the variables and therefore that is being tested in other methods of the controllers.


=== RSpec ===
No changes were made to the associated views and model file.
There were already quite a few test cases for the AssignmentsController. We have not added any new tests but made sure that no tests were failing due to our changes. No changes were made to the associated views and model file.


== References ==
== References ==
Line 96: Line 120:
3. http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2015/oss_E1553_AAJ <br/>
3. http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2015/oss_E1553_AAJ <br/>
4. https://dzone.com/articles/software-design-principles-dry-and-kiss <br/>
4. https://dzone.com/articles/software-design-principles-dry-and-kiss <br/>
5. https://en.wikipedia.org/wiki/Delegation_pattern

Latest revision as of 14:47, 2 April 2019

This wiki page is for the description of changes made under E1910 OSS assignment for Spring 2019, CSC/ECE 517.

We have not deployed this project since we did not change or add any functionality that would affect the user.

Expertiza Background

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. [3]

Description of the current project

The assignments_controller has multiple functions with wrong naming convention, they are classic examples of bad function names. Few of the variables names used in the method can also be refactored to make it more relevant and understandable. In addition, functions code can be optimized to ensure that it follows 'Don't Repeat Yourself' (DRY) principle. Also, most of the functions have missing code comments, which should be added to the functions.

Pull request link

[1]

Forked Github repo

[2]

Files modified in current project

The only controller modified for this project was the:
1. AssignmentsController
2. Assignment_helper

The updated method names in AssignmentController: 

 - handle_assignment_directory_path_nonexist_case_and_answer_tagging 
   -- missing_submission_directory
 - assignment_form_key_nonexist_case_handler 
   -- assignment_submission_handler
 - handle_current_user_timezonepref_nil 
   -- timezone_handler
 - update_nil_dd_deadline_name 
   -- update_due_date_deadline_name
 - update_nil_dd_description_url 
   -- update_due_date_description_url

 - assignment_form_assignment_staggered_deadline? 
   -- assignment_staggered_deadline?
 - check_due_date_nameurl_not_empty 
   -- check_due_date_nameurl

 - update_feedback_assignment_form_attributes 
   -- update_feedback_attributes


The thought process behind changing the method names was to make the names shorter and reduce redundancy. There were a few rules that we learned from our mentor; words like nil and nonexist are not allowed in method names. So for handle_current_user_timezonepref_nil we first removed the nil. Then after looking at what the method does we determined that timezone_handler would be appropriate and tells the user enough at a moments glance. More information is available about the method in the form of a block comment.

Another example is assignment_form_assignment_staggered_deadline?, the assignment_form bit is redundant, since it is the assignments controller the beginning part does not provide any new information and assignment_staggered_deadline? is sufficient.

A different kind of naming is update_nil_dd_deadline_name, we removed the nil but the dd doesn't make sense. No one would know what that stands for just by reading it. This is why this method was changed to update_due_date_deadline_name. It is not any shorter but much more useful at a glance.


The assignment_helper file was added to. Some of the methods in the controller were misplaced and according to the DRY principle these methods are better suited to be in the helper method. The methods that were moved from the controller to the helper were: 

 - assignment_submission_handler -> updated method name
 - check_due_date_nameurl(dd) -> updated method name


We were asked to refactor the action_allowed? method but after discussing some reafactoring methods with our mentor it was decided that refactoring this method would not promote the DRY principle and the methods created as a result would only be for that one purpose and therefore unnecessary.

Code Improvements

The DRY principle states, "Every piece of knowledge or logic must have a single, unambiguous representation within a system."[4] This project was about furthering this principle and keeping this controller concise and maintainable.


This is a controller that helps instructors, TA's, administrators, and super-administrators create/update/edit/show/delete current and past assignments. We updated the aforementioned method names so that the naming convention follows the DRY principle. Many of the method names were far too long, repetitive or unhelpful in understanding what the method actually does. In accordance with the principle we updated the method names to be simple while also being useful in letting the programmer know what the method does.


Many of these methods did not have any comments to help the programmer understand the logic driving the method. To further help the programmer in understanding what the code does we added block comments at the beginning of each method that we updated.


We cleaned up the controller by moving some methods from the controller into the associated helper file (assignment_helper.rb). By doing this we made the controller less messy and easier to read while maintaining and furthering the DRY principle. The code is now easily maintainable and all "helper" methods are in one place rather than separated across many files.


The delegation pattern states, "In delegation, an object handles a request by delegating to a second object (the delegate). The delegate is a helper object, but with the original context." [5]

As something extra, the create method was refactored so that the branch condition would be lower and there would be no repetition. We decided to use the delegation pattern because it serves the purpose of cleaning up existing methods while maintaining functionality. As per this pattern we took a request from the create method and put it in a helper method called update_assignment_form. This method handles updating the id of the assignment_questionnaire and the due_date and updating the form accordingly. The create method delegates this task to the update_assignment_form.

The update_assignment_form delegated a tasks to the helper method, array_traverser. This method does the job of going through the array and updates id fields to be strings instead of an integer.


As a result of these changes two code climate issues have been resolved which are stated below in the Code Climate section. Below are images of the way the create method used to look and how it looks now.

Old create method:


New create method:


Code Climate

Code Climate checks to make sure that the code that has been written follows certain rules so that everything is legible and concise. During our refactoring we have fixed the following code climate issues.

- The create method's Cognitive Complexity was too high. That has been resolved by the delegation we have described above.
- The create method was too long with 33 lines of code. That has been resolved by the delegation we have described above.
- The Branch Condition size for assignment_form_key_nonexist_case_handler, which is now assignment_submission_handler, was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code.
- The Branch Condition size for update_feedback_assignment_form_attributes, which is now update_feedback_attributes
, was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code.


We have three current code climate issues but after discussing with our mentor we have been told to ignore those.

Test Plan

The code coverage for the controller was 49% when we got the project. The code coverage when we finished was 53.32%.

There is a video on Expertiza titled "Rspec_test.mp4" which shows the updated tests running successfully.

RSpec Tests

There were already quite a few test cases for the AssignmentsController (assignments_controller_spec.rb). The create block tested just about everything. The two methods we added are private and since we branched out some functionality this needed to be a bit more explicitly tested. There is no precedence for testing private methods and it was frowned upon as per the video The Magic Tricks of Testing. We tested the public method, create, that calls the private method, update_assignment_form and array_traverser. We added a test with the context when assignment_form is saved successfully but fails to update.

For the other untested public methods, we found no usage directly in the controller, therefore, we did not test them. These methods are performing the tasks of assigning values to the variables and therefore that is being tested in other methods of the controllers.

No changes were made to the associated views and model file.

References

1. https://github.com/expertiza/expertiza/pull/1377
2. https://github.com/veehakhanna/expertiza
3. http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2015/oss_E1553_AAJ
4. https://dzone.com/articles/software-design-principles-dry-and-kiss
5. https://en.wikipedia.org/wiki/Delegation_pattern

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2019_E1910_Refactor_assignments_controller.rb&oldid=123148"