Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-12T04:18:15Z

Schawan: /* Approach */

==Fix issues related to deadlines and late policies==

This page gives a description of the issues and the approach towards solving them in the E1978. Fix issues related to deadlines and late policies project.

__TOC__

===Problem Description===
----
Expertiza can automatically deduct points if a student is late in performing some action (e.g., submitting or reviewing). This is implemented by defining a new “late policy” and applying it to the assignment. Late policies are managed on the Due dates tab of assignment creation (or editing).

 
====Issues with the Current Implementation====
----
The current implementation of Expertiza has the following Issues:
* '''Issue 1''' - In the UI, the policy is sometimes called a “late policy” and sometimes a “penalty policy”. To avoid confusion, use “late policy” exclusively.
* '''Issue 2''' - Late policies cannot be created without raising an error.
* '''Issue 3''' - After creating a late policy, the “back” link does not take the user back to editing the assignment.
* '''Issue 4''' - The "+" and "-" buttons for Show/Hide Date Updater do not work. Expertiza has a way of adjusting due dates for a new assignment. If a new assignment is created by copying an old assignment, the due dates are copied too. It is likely that sometimes the same increment between due dates as in the old assignment (e.g., two days for the review phase, three days for resubmission, etc.) is wished. To avoid the need to edit all the deadlines manually, Expertiza has a date updater. However, it does not work.
* '''Issue 5''' - Neither late_policy.rb nor late_policies_controller.rb have any tests at all.

[[File:Upload.png]]

 

===Project Scope===
----

Issues described in the above section all exist in the Due Dates tab on the Edit Assignment page.
* '''Issue 1''' - The scope of this issue is to change "penalty policy" to "late policy" on all the relevant UI screens (app/views/late_policies).
* '''Issue 2''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 3''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 4''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 5''' - The scope of this issue is to write test cases for late_policies_controller.rb and late_policy.rb.

===Approach===
----
 
[[File:Imageedit_5_7549932958.jpg]]

===Expected Results===
----
* '''Issue 1''' - Once this issue is fixed, the Due Dates tab while editing an assignment will have no ambiguity between "late policy" and "penalty policy", as "late policy" will be used exclusively.
* '''Issue 2''' - Once this issue is fixed, late policies can be created without raising the "The following error occurred while saving the penalty policy:" error.
* '''Issue 3''' - Once this issue is fixed, the back link after creating a late policy will rightly take the user to the Edit Assignment page.
* '''Issue 4''' - Once this issue is fixed, "+" and "-" buttons for Show/Hide Date Updater will work and update the dates as it is supposed to.
* '''Issue 5''' - The code coverage will definitely increase once tests for late_policy.rb and late_policies_controller.rb are written.
 

===Test Plan===
----
'''Automated Testing Using RSpec'''

We plan to create two new files, late_policy_spec.rb and late_policies_controller_spec.rb to write tests for late_policy.rb and late_policies_controller.rb repesctively.

'''Coverage'''

To make sure we are covering all of our code (100%) with test cases we will use the Ruby Code Coverage plugin of RubyMine and SimpleCov gem. After manually running the test file, the SimpleCov gem will highlight the lines of code which are being covered by our test cases in green color, and for those which are not being covered in red.

 

===Milestones===
----
'''11/01''' – Project Selection

'''11/11''' – Create Design Doc

'''11/15''' – setting up project environment and updating Design Doc

'''11/25''' – Fix Issues 1,2 and 3 with well-defined test cases

'''11/30''' – Fix issues 4 and 5 with well-defined test cases

'''12/04''' – Update design doc and create & upload video

'''12/06''' – Write tests for edge cases.

 

===Project Mentors===
----
* Edward Gehringer (efg@ncsu.edu)

* Ramya Vijayakumar (rvijaya4@ncsu.edu)

 

===Team Members===
----
* Rajit Bharambe (rbharam@ncsu.edu)

* Saahil Chawande (schawan@ncsu.edu)

* Natansh Negi (nnegi2@ncsu.edu)

* Hitarth Shah (hshah4@ncsu.edu)

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/rajitnb/expertiza GitHub Project Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-12T04:18:02Z

Schawan: /* Approach */

==Fix issues related to deadlines and late policies==

This page gives a description of the issues and the approach towards solving them in the E1978. Fix issues related to deadlines and late policies project.

__TOC__

===Problem Description===
----
Expertiza can automatically deduct points if a student is late in performing some action (e.g., submitting or reviewing). This is implemented by defining a new “late policy” and applying it to the assignment. Late policies are managed on the Due dates tab of assignment creation (or editing).

 
====Issues with the Current Implementation====
----
The current implementation of Expertiza has the following Issues:
* '''Issue 1''' - In the UI, the policy is sometimes called a “late policy” and sometimes a “penalty policy”. To avoid confusion, use “late policy” exclusively.
* '''Issue 2''' - Late policies cannot be created without raising an error.
* '''Issue 3''' - After creating a late policy, the “back” link does not take the user back to editing the assignment.
* '''Issue 4''' - The "+" and "-" buttons for Show/Hide Date Updater do not work. Expertiza has a way of adjusting due dates for a new assignment. If a new assignment is created by copying an old assignment, the due dates are copied too. It is likely that sometimes the same increment between due dates as in the old assignment (e.g., two days for the review phase, three days for resubmission, etc.) is wished. To avoid the need to edit all the deadlines manually, Expertiza has a date updater. However, it does not work.
* '''Issue 5''' - Neither late_policy.rb nor late_policies_controller.rb have any tests at all.

[[File:Upload.png]]

 

===Project Scope===
----

Issues described in the above section all exist in the Due Dates tab on the Edit Assignment page.
* '''Issue 1''' - The scope of this issue is to change "penalty policy" to "late policy" on all the relevant UI screens (app/views/late_policies).
* '''Issue 2''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 3''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 4''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 5''' - The scope of this issue is to write test cases for late_policies_controller.rb and late_policy.rb.

===Approach===
----
 
[[File:Imageedit_5_7549932958.png]]

===Expected Results===
----
* '''Issue 1''' - Once this issue is fixed, the Due Dates tab while editing an assignment will have no ambiguity between "late policy" and "penalty policy", as "late policy" will be used exclusively.
* '''Issue 2''' - Once this issue is fixed, late policies can be created without raising the "The following error occurred while saving the penalty policy:" error.
* '''Issue 3''' - Once this issue is fixed, the back link after creating a late policy will rightly take the user to the Edit Assignment page.
* '''Issue 4''' - Once this issue is fixed, "+" and "-" buttons for Show/Hide Date Updater will work and update the dates as it is supposed to.
* '''Issue 5''' - The code coverage will definitely increase once tests for late_policy.rb and late_policies_controller.rb are written.
 

===Test Plan===
----
'''Automated Testing Using RSpec'''

We plan to create two new files, late_policy_spec.rb and late_policies_controller_spec.rb to write tests for late_policy.rb and late_policies_controller.rb repesctively.

'''Coverage'''

To make sure we are covering all of our code (100%) with test cases we will use the Ruby Code Coverage plugin of RubyMine and SimpleCov gem. After manually running the test file, the SimpleCov gem will highlight the lines of code which are being covered by our test cases in green color, and for those which are not being covered in red.

 

===Milestones===
----
'''11/01''' – Project Selection

'''11/11''' – Create Design Doc

'''11/15''' – setting up project environment and updating Design Doc

'''11/25''' – Fix Issues 1,2 and 3 with well-defined test cases

'''11/30''' – Fix issues 4 and 5 with well-defined test cases

'''12/04''' – Update design doc and create & upload video

'''12/06''' – Write tests for edge cases.

 

===Project Mentors===
----
* Edward Gehringer (efg@ncsu.edu)

* Ramya Vijayakumar (rvijaya4@ncsu.edu)

 

===Team Members===
----
* Rajit Bharambe (rbharam@ncsu.edu)

* Saahil Chawande (schawan@ncsu.edu)

* Natansh Negi (nnegi2@ncsu.edu)

* Hitarth Shah (hshah4@ncsu.edu)

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/rajitnb/expertiza GitHub Project Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]

File:Imageedit 5 7549932958.jpg

2019-11-12T04:17:16Z

Schawan:

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T03:06:38Z

Schawan: /* Test Plan */

==Fix issues related to deadlines and late policies==

This page gives a description of the issues and the approach towards solving them in the E1978. Fix issues related to deadlines and late policies project.

__TOC__

===Problem Description===
----
Expertiza can automatically deduct points if a student is late in performing some action (e.g., submitting or reviewing). This is implemented by defining a new “late policy” and applying it to the assignment. Late policies are managed on the Due dates tab of assignment creation (or editing).

 
====Issues with the Current Implementation====
----
The current implementation of Expertiza has the following Issues:
* '''Issue 1''' - In the UI, the policy is sometimes called a “late policy” and sometimes a “penalty policy”. To avoid confusion, use “late policy” exclusively.
* '''Issue 2''' - Late policies cannot be created without raising an error.
* '''Issue 3''' - After creating a late policy, the “back” link does not take the user back to editing the assignment.
* '''Issue 4''' - The "+" and "-" buttons for Show/Hide Date Updater do not work. Expertiza has a way of adjusting due dates for a new assignment. If a new assignment is created by copying an old assignment, the due dates are copied too. It is likely that sometimes the same increment between due dates as in the old assignment (e.g., two days for the review phase, three days for resubmission, etc.) is wished. To avoid the need to edit all the deadlines manually, Expertiza has a date updater. However, it does not work.
* '''Issue 5''' - Neither late_policy.rb nor late_policies_controller.rb have any tests at all.

[[File:Upload.png]]

 

===Project Scope===
----

Issues described in the above section all exist in the Due Dates tab on the Edit Assignment page.
* '''Issue 1''' - The scope of this issue is to change "penalty policy" to "late policy" on all the relevant UI screens (app/views/late_policies).
* '''Issue 2''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 3''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 4''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 5''' - The scope of this issue is to write test cases for late_policies_controller.rb and late_policy.rb.

===Approach===
----
 

===Expected Results===
----
* '''Issue 1''' - Once this issue is fixed, the Due Dates tab while editing an assignment will have no ambiguity between "late policy" and "penalty policy", as "late policy" will be used exclusively.
* '''Issue 2''' - Once this issue is fixed, late policies can be created without raising the "The following error occurred while saving the penalty policy:" error.
* '''Issue 3''' - Once this issue is fixed, the back link after creating a late policy will rightly take the user to the Edit Assignment page.
* '''Issue 4''' - Once this issue is fixed, "+" and "-" buttons for Show/Hide Date Updater will work and update the dates as it is supposed to.
* '''Issue 5''' - The code coverage will definitely increase once tests for late_policy.rb and late_policies_controller.rb are written.
 

===Test Plan===
----
'''Automated Testing Using RSpec'''

We plan to create two new files, late_policy_spec.rb and late_policies_controller_spec.rb to write tests for late_policy.rb and late_policies_controller.rb repesctively.

'''Coverage'''

To make sure we are covering all of our code (100%) with test cases we will use the Ruby Code Coverage plugin of RubyMine and SimpleCov gem. After manually running the test file, the SimpleCov gem will highlight the lines of code which are being covered by our test cases in green color, and for those which are not being covered in red.

 

===Milestones===
----
'''11/01''' – Project Selection

'''11/11''' – Create Design Doc

'''11/15''' – setting up project environment and updating Design Doc

'''11/25''' – Fix Issues 1,2 and 3 with well-defined test cases

'''11/30''' – Fix issues 4 and 5 with well-defined test cases

'''12/04''' – Update design doc and create & upload video

'''12/06''' – Write tests for edge cases.

 

===Project Mentors===
----
* Edward Gehringer (efg@ncsu.edu)

* Ramya Vijayakumar (rvijaya4@ncsu.edu)

 

===Team Members===
----
* Rajit Bharambe (rbharam@ncsu.edu)

* Saahil Chawande (schawan@ncsu.edu)

* Natansh Negi (nnegi2@ncsu.edu)

* Hitarth Shah (hshah4@ncsu.edu)

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/rajitnb/expertiza GitHub Project Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T03:06:13Z

Schawan: /* Test Plan */

==Fix issues related to deadlines and late policies==

This page gives a description of the issues and the approach towards solving them in the E1978. Fix issues related to deadlines and late policies project.

__TOC__

===Problem Description===
----
Expertiza can automatically deduct points if a student is late in performing some action (e.g., submitting or reviewing). This is implemented by defining a new “late policy” and applying it to the assignment. Late policies are managed on the Due dates tab of assignment creation (or editing).

 
====Issues with the Current Implementation====
----
The current implementation of Expertiza has the following Issues:
* '''Issue 1''' - In the UI, the policy is sometimes called a “late policy” and sometimes a “penalty policy”. To avoid confusion, use “late policy” exclusively.
* '''Issue 2''' - Late policies cannot be created without raising an error.
* '''Issue 3''' - After creating a late policy, the “back” link does not take the user back to editing the assignment.
* '''Issue 4''' - The "+" and "-" buttons for Show/Hide Date Updater do not work. Expertiza has a way of adjusting due dates for a new assignment. If a new assignment is created by copying an old assignment, the due dates are copied too. It is likely that sometimes the same increment between due dates as in the old assignment (e.g., two days for the review phase, three days for resubmission, etc.) is wished. To avoid the need to edit all the deadlines manually, Expertiza has a date updater. However, it does not work.
* '''Issue 5''' - Neither late_policy.rb nor late_policies_controller.rb have any tests at all.

[[File:Upload.png]]

 

===Project Scope===
----

Issues described in the above section all exist in the Due Dates tab on the Edit Assignment page.
* '''Issue 1''' - The scope of this issue is to change "penalty policy" to "late policy" on all the relevant UI screens (app/views/late_policies).
* '''Issue 2''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 3''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 4''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 5''' - The scope of this issue is to write test cases for late_policies_controller.rb and late_policy.rb.

===Approach===
----
 

===Expected Results===
----
* '''Issue 1''' - Once this issue is fixed, the Due Dates tab while editing an assignment will have no ambiguity between "late policy" and "penalty policy", as "late policy" will be used exclusively.
* '''Issue 2''' - Once this issue is fixed, late policies can be created without raising the "The following error occurred while saving the penalty policy:" error.
* '''Issue 3''' - Once this issue is fixed, the back link after creating a late policy will rightly take the user to the Edit Assignment page.
* '''Issue 4''' - Once this issue is fixed, "+" and "-" buttons for Show/Hide Date Updater will work and update the dates as it is supposed to.
* '''Issue 5''' - The code coverage will definitely increase once tests for late_policy.rb and late_policies_controller.rb are written.
 

===Test Plan===
----

'''Automated Testing Using RSpec'''

We plan to create two new files, late_policy_spec.rb and late_policies_controller_spec.rb to write tests for late_policy.rb and late_policies_controller.rb repesctively.

'''Coverage'''

To make sure we are covering all of our code (100%) with test cases we will use the Ruby Code Coverage plugin of RubyMine and SimpleCov gem. After manually running the test file, the SimpleCov gem will highlight the lines of code which are being covered by our test cases in green color, and for those which are not being covered in red.

 

===Milestones===
----
'''11/01''' – Project Selection

'''11/11''' – Create Design Doc

'''11/15''' – setting up project environment and updating Design Doc

'''11/25''' – Fix Issues 1,2 and 3 with well-defined test cases

'''11/30''' – Fix issues 4 and 5 with well-defined test cases

'''12/04''' – Update design doc and create & upload video

'''12/06''' – Write tests for edge cases.

 

===Project Mentors===
----
* Edward Gehringer (efg@ncsu.edu)

* Ramya Vijayakumar (rvijaya4@ncsu.edu)

 

===Team Members===
----
* Rajit Bharambe (rbharam@ncsu.edu)

* Saahil Chawande (schawan@ncsu.edu)

* Natansh Negi (nnegi2@ncsu.edu)

* Hitarth Shah (hshah4@ncsu.edu)

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/rajitnb/expertiza GitHub Project Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T03:05:50Z

Schawan:

==Fix issues related to deadlines and late policies==

This page gives a description of the issues and the approach towards solving them in the E1978. Fix issues related to deadlines and late policies project.

__TOC__

===Problem Description===
----
Expertiza can automatically deduct points if a student is late in performing some action (e.g., submitting or reviewing). This is implemented by defining a new “late policy” and applying it to the assignment. Late policies are managed on the Due dates tab of assignment creation (or editing).

 
====Issues with the Current Implementation====
----
The current implementation of Expertiza has the following Issues:
* '''Issue 1''' - In the UI, the policy is sometimes called a “late policy” and sometimes a “penalty policy”. To avoid confusion, use “late policy” exclusively.
* '''Issue 2''' - Late policies cannot be created without raising an error.
* '''Issue 3''' - After creating a late policy, the “back” link does not take the user back to editing the assignment.
* '''Issue 4''' - The "+" and "-" buttons for Show/Hide Date Updater do not work. Expertiza has a way of adjusting due dates for a new assignment. If a new assignment is created by copying an old assignment, the due dates are copied too. It is likely that sometimes the same increment between due dates as in the old assignment (e.g., two days for the review phase, three days for resubmission, etc.) is wished. To avoid the need to edit all the deadlines manually, Expertiza has a date updater. However, it does not work.
* '''Issue 5''' - Neither late_policy.rb nor late_policies_controller.rb have any tests at all.

[[File:Upload.png]]

 

===Project Scope===
----

Issues described in the above section all exist in the Due Dates tab on the Edit Assignment page.
* '''Issue 1''' - The scope of this issue is to change "penalty policy" to "late policy" on all the relevant UI screens (app/views/late_policies).
* '''Issue 2''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 3''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 4''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 5''' - The scope of this issue is to write test cases for late_policies_controller.rb and late_policy.rb.

===Approach===
----
 

===Expected Results===
----
* '''Issue 1''' - Once this issue is fixed, the Due Dates tab while editing an assignment will have no ambiguity between "late policy" and "penalty policy", as "late policy" will be used exclusively.
* '''Issue 2''' - Once this issue is fixed, late policies can be created without raising the "The following error occurred while saving the penalty policy:" error.
* '''Issue 3''' - Once this issue is fixed, the back link after creating a late policy will rightly take the user to the Edit Assignment page.
* '''Issue 4''' - Once this issue is fixed, "+" and "-" buttons for Show/Hide Date Updater will work and update the dates as it is supposed to.
* '''Issue 5''' - The code coverage will definitely increase once tests for late_policy.rb and late_policies_controller.rb are written.
 

===Test Plan===
----

'''Automated Testing Using RSpec'''

We plan to create two new files, late_policy_spec.rb and late_policies_controller_spec.rb to write tests for late_policy.rb and late_policies_controller.rb repesctively.

'''Coverage'''

To make sure we are covering all of our code (100%) with test cases we will use the Ruby Code Coverage plugin of RubyMine and SimpleCov gem. After manually running the test file, the SimpleCov gem will highlight the lines of code which are being covered by our test cases in green color, and for those which are not being covered in red.
 

===Milestones===
----
'''11/01''' – Project Selection

'''11/11''' – Create Design Doc

'''11/15''' – setting up project environment and updating Design Doc

'''11/25''' – Fix Issues 1,2 and 3 with well-defined test cases

'''11/30''' – Fix issues 4 and 5 with well-defined test cases

'''12/04''' – Update design doc and create & upload video

'''12/06''' – Write tests for edge cases.

 

===Project Mentors===
----
* Edward Gehringer (efg@ncsu.edu)

* Ramya Vijayakumar (rvijaya4@ncsu.edu)

 

===Team Members===
----
* Rajit Bharambe (rbharam@ncsu.edu)

* Saahil Chawande (schawan@ncsu.edu)

* Natansh Negi (nnegi2@ncsu.edu)

* Hitarth Shah (hshah4@ncsu.edu)

===References===
#[https://github.com/expertiza/expertiza Expertiza on GitHub]

#[https://github.com/rajitnb/expertiza GitHub Project Fork]

#[http://expertiza.ncsu.edu/ The live Expertiza website]

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T03:02:16Z

Schawan: /* Project Mentors */

==Fix issues related to deadlines and late policies==

This page gives a description of the issues and the approach towards solving them in the E1978. Fix issues related to deadlines and late policies project.

__TOC__

===Problem Description===
----
Expertiza can automatically deduct points if a student is late in performing some action (e.g., submitting or reviewing). This is implemented by defining a new “late policy” and applying it to the assignment. Late policies are managed on the Due dates tab of assignment creation (or editing).

 
====Issues with the Current Implementation====
----
The current implementation of Expertiza has the following Issues:
* '''Issue 1''' - In the UI, the policy is sometimes called a “late policy” and sometimes a “penalty policy”. To avoid confusion, use “late policy” exclusively.
* '''Issue 2''' - Late policies cannot be created without raising an error.
* '''Issue 3''' - After creating a late policy, the “back” link does not take the user back to editing the assignment.
* '''Issue 4''' - The "+" and "-" buttons for Show/Hide Date Updater do not work. Expertiza has a way of adjusting due dates for a new assignment. If a new assignment is created by copying an old assignment, the due dates are copied too. It is likely that sometimes the same increment between due dates as in the old assignment (e.g., two days for the review phase, three days for resubmission, etc.) is wished. To avoid the need to edit all the deadlines manually, Expertiza has a date updater. However, it does not work.
* '''Issue 5''' - Neither late_policy.rb nor late_policies_controller.rb have any tests at all.

[[File:Upload.png]]

 

===Project Scope===
----

Issues described in the above section all exist in the Due Dates tab on the Edit Assignment page.
* '''Issue 1''' - The scope of this issue is to change "penalty policy" to "late policy" on all the relevant UI screens (app/views/late_policies).
* '''Issue 2''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 3''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 4''' - The scope of this issue is to fix the function in ruby and write appropriate test cases (late_policies_controller.rb).
* '''Issue 5''' - The scope of this issue is to write test cases for late_policies_controller.rb and late_policy.rb.

Issues described in the above section all exist in the Due Dates tab on the Edit Assignment page.
* '''Issue 1''' - Once this issue is fixed, the Due Dates tab while editing an assignment will have no ambiguity between "late policy" and "penalty policy", as "late policy" will be used exclusively.
* '''Issue 2''' - Once this issue is fixed, late policies can be created without raising the "The following error occurred while saving the penalty policy:" error.
* '''Issue 3''' - Once this issue is fixed, the back link after creating a late policy will rightly take the user to the Edit Assignment page.
* '''Issue 4''' - Once this issue is fixed, "+" and "-" buttons for Show/Hide Date Updater will work and update the dates as it is supposed to.
* '''Issue 5''' - The code coverage will definitely increase once tests for late_policy.rb and late_policies_controller.rb are written.
 

===Approach===
----
 

===Test Plan===
----

'''Automated Testing Using RSpec'''

We plan to create two new files, late_policy_spec.rb and late_policies_controller_spec.rb to write tests for late_policy.rb and late_policies_controller.rb repesctively.

'''Coverage'''

To make sure we are covering all of our code (100%) with test cases we will use the Ruby Code Coverage plugin of RubyMine and SimpleCov gem. After manually running the test file, the SimpleCov gem will highlight the lines of code which are being covered by our test cases in green color, and for those which are not being covered in red.
 

===Milestones===
----
'''11/01''' – Project Selection

'''11/11''' – Create Design Doc

'''11/15''' – setting up project environment and updating Design Doc

'''11/25''' – Fix Issues 1,2 and 3 with well-defined test cases

'''11/30''' – Fix issues 4 and 5 with well-defined test cases

'''12/04''' – Update design doc and create & upload video

'''12/06''' – Write tests for edge cases.

 

===Project Mentors===
----
* Edward Gehringer (efg@ncsu.edu)

* Ramya Vijayakumar (rvijaya4@ncsu.edu)

 

===Team Members===
----
1. Rajit Bharambe (rbharam@ncsu.edu)

2. Saahil Chawande (schawan@ncsu.edu)

3. Natansh Negi (nnegi2@ncsu.edu)

4. Hitarth Shah (hshah4@ncsu.edu)

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T02:23:38Z

Schawan: /* Scope of the Project */

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T02:23:21Z

Schawan: /* Issues with the Current Implementation */

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T02:23:03Z

Schawan: /* Issues with the Current Implementation */

CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies

2019-11-11T02:22:45Z

Schawan: /* Test Plan */

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T23:07:19Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png]]
----
[[File:Screenshoot_PR.png]]

 

===Results/Conclusions===
----
After refactoring the code, following is achieved:
#Made code more readable
#Cleaned up the code and made it tidier
#Removed redundant, unused code and comments
#Improved performance
#Made some things more generic (like get_status() method)
#Kept the code follow the DRY principle (delete_signup and delete_signup_as_instructor now satisfy the DRY principle)
#Combined and disposed "Like" or "Similar" code.
#Splitted out long functions into more manageable bite.
#Created reusable code
#Made the class and function cohesion better.

 

===Project Mentors===
----
#Edward Gehringer (efg@ncsu.edu)
#Ramya Vijayakumar (rvijaya4@ncsu.edu)
 

===Team Members===
----
#Rajit Bharambe (rbharam@ncsu.edu)
#Saahil Chawande (schawan@ncsu.edu)
#Sravan Matta (skmatta@ncsu.edu)
 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T21:23:46Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png]]
----
[[File:Screenshoot_PR.png]]

 

===Project Mentors===
----
#Edward Gehringer (efg@ncsu.edu)
#Ramya Vijayakumar (rvijaya4@ncsu.edu)
 

===Team Members===
----
#Rajit Bharambe (rbharam@ncsu.edu)
#Saahil Chawande (schawan@ncsu.edu)
#Sravan Matta (skmatta@ncsu.edu)
 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

File:Screenshoot PR.png

2019-11-06T21:23:12Z

Schawan: uploaded a new version of "File:Screenshoot PR.png"

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T21:21:42Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png]]
[[File:Screenshoot_PR.png]]

 

===Project Mentors===
----
#Edward Gehringer (efg@ncsu.edu)
#Ramya Vijayakumar (rvijaya4@ncsu.edu)
 

===Team Members===
----
#Rajit Bharambe (rbharam@ncsu.edu)
#Saahil Chawande (schawan@ncsu.edu)
#Sravan Matta (skmatta@ncsu.edu)
 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

File:Screenshoot PR.png

2019-11-06T21:20:59Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T21:14:31Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png]]

 

===Project Mentors===
----
#Edward Gehringer (efg@ncsu.edu)
#Ramya Vijayakumar (rvijaya4@ncsu.edu)
 

===Team Members===
----
#Rajit Bharambe (rbharam@ncsu.edu)
#Saahil Chawande (schawan@ncsu.edu)
#Sravan Matta (skmatta@ncsu.edu)
 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

File:Screenshot.png

2019-11-06T20:51:14Z

Schawan: uploaded a new version of "File:Screenshot.png"

The page we going to export to PDF

File:Screenshot.png

2019-11-06T20:43:42Z

Schawan: uploaded a new version of "File:Screenshot.png"

The page we going to export to PDF

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:41:49Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

File:Screenshot.png

2019-11-06T20:41:25Z

Schawan: uploaded a new version of "File:Screenshot.png"

The page we going to export to PDF

File:Screenshot.png

2019-11-06T20:40:16Z

Schawan: uploaded a new version of "File:Screenshot.png"

The page we going to export to PDF

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:35:46Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png|200]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:35:22Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png|200px]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:34:21Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png|Width100xHeight100]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:34:01Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png|Width200xHeight200]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:32:48Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:32:27Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png|200px|left|alt text]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:31:51Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png|200px|thumb|left|alt text]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:30:06Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 
[[File:Screenshot.png]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

File:Screenshot.png

2019-11-06T20:29:10Z

Schawan: uploaded a new version of "File:Screenshot.png"

The page we going to export to PDF

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:27:06Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
 

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T20:26:38Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----
'''Testing with Travis CI'''
[[File:Example.jpg]]

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T18:37:05Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>
 

===Testing===
----

 

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T18:36:16Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end

def get_status(status_for, participant, assignment, drop_topic_deadline)
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
end
if status_for == "instructor"
return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
end
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop a student after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
end
if status_for == "instructor"
return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
end
else
delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
flash[:success] = "You have successfully dropped the student from the topic!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
if status_for == "student"
return {:type => "success", :message => "You have successfully dropped your topic!"}
end
if status_for == "instructor"
return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
end
end
redirect_to controller: 'assignments', action: 'edit', id: assignment.id
end
</pre>

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T18:34:59Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end
</pre>

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T18:31:56Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
flash_message = get_status("student", participant, assignment, drop_topic_deadline)
if flash_message[:type] == "error"
flash[:error] = flash_message[:message]
else
end
redirect_to action: 'list', id: params[:id]
end
</pre>

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T18:30:36Z

Schawan:

==Refactor sign up sheet controller.rb==

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

('''NOTE''': This wiki is not final and is still a work in progress.)

__TOC__

===About Expertiza===
----

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.

 

===Description of the project===
----

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

 

===About Signup Sheet Controller===
----
This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

 

===Issues and Solutions===
----

* '''Issue 1''' - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
**'''Solution'''- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
 
* '''Issue 2''' - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
**'''Solution'''-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.
 
* '''Issue 3''' - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
**'''Solution'''-Used a single update_attributes method in place of the many instance variables.
 
* '''Issue 4''' - Destroy has a misleading else flash message.
**'''Solution'''-Changed the misleading flash message in the else condition to be more straightforward.
 
* '''Issue 5''' - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
***'''Solution'''-It was found that the method add_signup_topics_staggered was unnecessary and was removed.
 
* '''Issue 6''' - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
**'''Solution'''-Several method names were appropriately improved and renamed.
 
* '''Issue 7''' - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
**'''Solution'''-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.
 
* '''Issue 8''' - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
**'''Solution'''-Provided comments for blocks of code and created helper methods.
 
* '''Issue 9''' - Refactor participants variable in load_add_signup_topics
**'''Solution'''-Refactored the participants variable and improved readability.
 
* '''Issue 10''' - Signup_as_instructor_action has a complicated if-else ladder.
**'''Solution'''-The if-else ladder was made more elegant and readable.
 
* '''Issue 11''' - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
**'''Solution'''-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

 

===Code Improvements===
----
* The '''create''' method was responsible for calling the update method directly. Generally the create method should not directly call the update method.
'''Before'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 
'''After'''
<pre>
def create
topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
call_create_or_update(topic)
end

# This method appropriately calls the create topic or update existing topic method
def call_create_or_update(topic)
if topic.nil?
setup_new_topic
else
update_existing_topic topic
end
end
</pre>
 

* The '''update''' method has a lot of instance variables initialized. The method has been changed in the following ways
'''Before'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>

'''After'''
<pre>
def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_max_choosers @topic
updated_max_choosers = @topic.max_choosers
@topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = "The topic could not be updated."
end
end
</pre>
 

* Fixed the if-else ladder in '''signup_as_instructor_action''' action:
'''Before'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
else
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
end
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 
'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>
 

*The '''delete_signup''' method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.
'''Before'''
<pre>
def delete_signup
participant = AssignmentParticipant.find(params[:id])
assignment = participant.assignment
drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
# A student who has already submitted work should not be allowed to drop his/her topic!
# (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
# If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
# If there is a drop topic deadline, student cannot drop topic after this deadline.
if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
flash[:error] = "You cannot drop your topic after the drop topic deadline!"
ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
else
delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
flash[:success] = "You have successfully dropped your topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
end
redirect_to action: 'list', id: params[:id]
end
</pre>

'''After'''
<pre>
def signup_as_instructor_action
user = User.find_by(name: params[:username])
if user.nil? # validate invalid user
flash[:error] = "That student does not exist!"
else
if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
flash[:success] = "You have successfully signed up the student for the topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
flash[:error] = "The student has already signed up for a topic!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
else
flash[:error] = "The student is not registered for the assignment!"
ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
end
end
redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
</pre>

===References===
----
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/saahilchawande/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
 
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T18:20:29Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:48:56Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:23:26Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:19:22Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:19:03Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:18:06Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:16:53Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:15:18Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:07:28Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:06:59Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:06:47Z

Schawan:

CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

2019-11-06T17:06:08Z

Schawan: