CSC/ECE 517 Fall 2013/ch2 0e808 nsv

E808: Refactor and test Participant, AssignmentParticipant, & CourseParticipant

The requirements provided for the Open Source System project were as follows :
Classes:
participant.rb (197 lines)
assignment_participant.rb (476 lines)
course_participant.rb (90 lines)

What they do:
These classes keep track of a user who is participating in an assignment or a course. participant.rb is the superclass, and the other two are the subclasses.

What is needed:
The methods are complex; there are a lot of them (assignment_particpant has 44) and there is a lot of duplicated code. assignment_participant contains code for detecting cycles of high grades (which can indicate collusion in reviews). It would be better moved to another class.

Introduction

A cursory analysis of the code of the three model classes showed the existence of code duplication. In a lot of the cases the Don't Repeat Yourself (DRY) principle was violated.

In keeping with the Agile methodology, which involves having regular discussions with all stakeholders, the design check for the project was performed with a meeting with the Instructor. This helped set a direction to the way ahead for the project. The process followed in detail is explained in the following sections.

Project Design and Approach

Since the Agile Method of software development was adopted for this project, the first basic principle was accepting the fact that not all the requirements would be known at the onset, but instead would change with time. Also the primary purpose of the project was to get acquainted with the techniques needed to understand a huge application already in place and coded by others.
With this in mind, there was the freedom to propose an array of changes and then through trial and error perceive their feasibility and impact on the application.
During the first design check with the Instructor, all the methods in the three models were parsed through and the existing issues were identified. What was also discussed was functionality that could be improved upon or new features that could be added but only if these improved the overall quality of the code or made better design sense.
The result of this meeting was a spreadsheet with all model methods, their functions and proposed changes. This spreadsheet was the used as the baseline for the entire project Following through with getting timely feedback from all stakeholders, this proposal was discussed with the Instructor and modified as per comments received. With the first set of requirements ready, the code could now be worked upon.
With the goal being refactoring and testing of the models, before any code changes could be made the existing tests needed to be verified to run successfully. Unit tests deal with models. Therefore the test files participant_test.rb, assignment_participant_test.rb and course_participant_test.rb were run , but found to have bugs. These were fixed (explained in detail in sections below). The only test that still failed was in assignment_participant_test.rb and dealt with the publishing rights functionality which is not yet working in the expertiza project. Hence this test's failure was expected behavior and thus ignored. Certain basic guidelines followed for improving the code design were :

1. Re-use code as much as possible
2. Make the code DRY and remove code duplication
3. Remove redundant or unnecessary methods
4. Redesign the code to make it more object oriented
5. Due to the hierarchy structure of the models, common functionality between the models needed to be moved to the base class
6. Specialized methods not relevant to the super-class should be moved to the sub-class it applies to
7. Any deprecated code must be modified in accordance with the new norms to ensure smooth running of code in future versions of Rails<ref>http://stackoverflow.com/questions/15098961/findfirst-and-findall-are-deprecated</ref>
8. Rename methods to conform better with Ruby naming conventions
9. Improve the readability of the code

UML Class Diagram

A subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

Participant is the superclass for subclasses AssignmentParticipant and CourseParticipant. In the database to actually distinguish between these a type attribute is used which can take values as 'AssignmentParticipant' or 'CourseParticipant' while a participant that does not belong to either an assignment nor a course cannot exist in the system as he just becomes a user and not a participant.
A participant has a user i.e. only a user can become a participant.
An AssignmentParticipant has an assignment and a CourseParticipant has a course. This relationship is defined by the parent_id attribute of a participant.

Participant model: participant.rb

The Participant model class being the base class should only contain code or methods that are relevant or needed by both the subclasses. But instead , it was cluttered with methods that would be better placed in one of the child classes as they are relevant to only that subclass. Such methods were moved to the respective sub-classes. Also certain functionalities that were common between both subclasses were repeated in both leading to code repetition. Such methods were moved to Participant class and will be dealt with in the later sections of this article. Several methods in Participant model required renaming and refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming convention. Deprecated code was removed and re-coded to ensure that the code worked in future versions of Rails as well.

Sr. No.	Method Name	Changes Made	Reason For Change
1	able_to_submit	It has been commented out.	Method not called or used anywhere. Also the method just returned the value of the attribute submit_allowed which is already taken care of by attr_accessors.
2	force_delete	The deprecated code in this method has been changed.	To ensure the code works in future versions of Rails.
3	get_average_question_score	Renamed to average_question_score and refactored.	To conform to method naming convention.
4	get_average_score	Renamed to average_score and refactored also moved to the model assignment_participant.rb. Removed empty parentheses from declaration	The new name is more in keeping with method naming conventions. Removal of empty parentheses helps with poetry mode of code. The method was moved because calculation of average score is only relevant to AssignmentParticipant and not to CourseParticipant.
5	get_current_stage	Moved to the model - assignment_participants.rb	The method was moved because getting current stage of an assignment is only relevant to AssignmentParticipant and not to CourseParticipant.
6	get_stage_deadline	Moved to model - assignment_participant.rb	Just like get_current_stage, this method too is only relevant to AssignmentParticipant and not to CourseParticipant.
7	get_topic_string	Renamed to topic_string and refactored.	To follow method naming convention
8	review_response_maps	Moved to model - assignment_participant.rb	Reviews are made only on assignments and hence ReviewResponseMaps would only exist for between the participants of an assignment. Thus this method would be better placed in AssignmentParticipant.

After changes were made, the existing tests ran successfully.

AssignmentParticipant model: assignment_participant.rb

Several methods in AssignmentParticipant model class required renaming and refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate the collusion in reviews between students were moved to a newly created model class called CollusionCycle in file collusion_cycle.rb .

Sr. No.	Method Name	Changes Made	Reason For Change
1	get_course_string	Renamed to course_string and refactored	To follow method naming convention
2	get_cycle_deviation_score	Moved to collusion_cycle.rb model class	Related methods that calculate collusion in reviews have been moved to the newly created collusion_cycle.rb model class
3	get_cycle_similarity_score	Moved to collusion_cycle.rb model class	Related methods that calculate collusion in reviews have been moved to the newly created collusion_cycle.rb model class
4	get_feedback	Renamed to feedback and refactored	To follow method naming convention
5	get_files	Renamed to files_in_directory and refactored	To follow method naming convention
6	get_four_node_cycles	Moved to collusion_cycle.rb model class	Related methods that calculate collusion in reviews have been moved to the newly created collusion_cycle.rb model class
7	fullname	Moved to parent model class – participant.rb	General methods of the subclass are moved to Parent class
8	get_metareviews	Renamed to metareviews and refactored	To follow method naming convention
9	name	Commented the method	The method is already in parent model class – participant.rb and can be used in this subclass through inheritance
10	name	Commented the method	The method is already in parent model class – participant.rb and can be used in this subclass through inheritance. Also it is a duplicate method in patricipant.rb
11	get_review_score	Renamed to review_score and refactored. Deprecated code has been removed and replaced with correct code.	To follow method naming convention and ensure the code works in future versions.
12	get_reviewees	Renamed to reviewees and refactored. Deprecated code has been removed and replaced with correct code.	To follow method naming convention and ensure the code works in future versions.
13	get_reviewers	Renamed to reviewers and refactored . Deprecated code has been commented and replaced with correct code.	To follow method naming convention and ensure the code works in future versions.
14	get_reviews	Renamed to reviews and refactored	To follow method naming convention
15	get_reviews_by_reviewer	Renamed to reviews_by_reviewer and refactored	To follow method naming convention
16	get_reviews_by_reviewer	Commented the method	To remove duplicated method in this class
17	get_scores	Renamed to scores and refactored	To follow method naming convention
18	get_submitted_files	Renamed to submitted_files and refactored	To follow method naming convention
19	get_teammate_reviews	Renamed to teammate_reviews and refactored	To follow method naming convention
20	get_three_node_cycles	Moved to collusion_cycle.rb model class	Related methods that calculate collusion in reviews have been moved to the newly created collusion_cycle.rb model class
21	get_topic_string	Method has been Commented	It is already present in model super class – participant.rb
22	get_two_node_cycles	Moved to collusion_cycle.rb model class	Related methods that calculate collusion in reviews have been moved to the newly created collusion_cycle.rb model class
23	get_wiki_submissions	Renamed to wiki_submissions and refactored	To follow method naming convention

After changes were made, the existing tests ran successfully.

CourseParticipant model:course_participant.rb

The CourseParticipant model has methods like get_path and self.get_export_fields which were present in both assignment_participant.rb and course_participant.rb. Since they had dynamic usages, we could not delete them from the two subclasses and place in participant.rb.

Sr. No.	Method Name	Changes Made	Reason For Change
1	get_course_string	Renamed to course_string and refactored	To follow method naming convention.

Changes to yaml files

Sr. No.	Method Name	Changes Made	Reason For Change
1	invitation.yml	The indentation was corrected.	Improper Indentation in invitation.yml was throwing errors.
2	users.yml	1.The symbol ‘:password’ was renamed to ‘:crypted_password’ 2.‘fullname :last,first’ was added inside ‘student1:’	1.Attribute name is crypted_password in user.rb 2.Fixture was missing definition of fullname which required for the tests.

Testing

course_participant_test.rb

Changed the following line :

CourseParticipant.import(['luke','Luke Skywalker','luke@gmail.com','darthvader'],{:user=>users(:superadmin)},courses(:course_e_commerce).id)

to

CourseParticipant.import(['luke','Skywalker,Luke','luke@gmail.com','darthvader'],{:user=>users(:superadmin)},courses(:course_e_commerce).id)

This was done to follow the syntax of fullname i.e.last_name,first_name which is a requirement of the application.

Rspec tests

participant_spec.rb

Created to check basic functionality of paticipant.rb Checked for:
1.Associations with other tables.
2.Entry and update of records.

Changes in routes.rb

The following missing routes were added to the file routes.rb to make the program flow smoothly.

Routes for submit_hyperlink and remove_hyperlink were added to the submitted_content controller as :

resources :submitted_content do
   collection do
     get :view
     get :edit
     post :submit_hyperlink
     get :remove_hyperlink
   end
 end

Routes for the get and post methods of the export_file controller were added :

resources :export_file do
   collection do
     get :start
     get :export
     post :export
   end
 end

Routes for cancel, accept and decline were added to the invitation controller :

 resources :invitation do
   collection do
     get :cancel
     get :accept
     get :decline
   end
 end

Scope for future work

• The tests related to ‘publishing_rights’ method in assignment_participant_test.rb still fail, but this is expected as this feature of publishing_rights has not yet been implemented in Expertiza.
• The delete hyperlink method needs to work as currently this feature is available in the running Expertiza.
• A test for cycle_collusion.rb as proposed:

class CycleCollusionTest < ActiveSupport::TestCase
	fixtures :response_maps, :users , :assignments
          test "collusion" do
          r = ResponseMap.new
          r.reviewee_id= response_maps(:response_maps0)  //his work was reviewed by user1
          r.reviewer_id= response_maps(:response_maps1) //he reviewed user0's work
          r.reviewee_id1=response_maps(:response_maps1)  //his work was reviewed by user0 
          r.reviewer_id1=response_maps(:response_maps0)  //he reviewed user1's work
          !assert_equal r.reviewee_id, r.reviewer_id1
          end

Could not be tested effectively as cycle_collusion is not used anywhere in the code except in assignment_participant.rb where the methods now belonging to cycle_collusion class were originally present.

• The feature of sending an email when a user gets registered for an assignment i.e.becomes an assignment participant needs to be implemented.

Conclusion

• Renaming and refactoring was performed as per requirements and to follow conventions for the appropriate methods in participant.rb, assignment_participant.rb and course_participant.rb.
• Bugs in existing tests of model classes’ participant.rb, assignment_participant.rb and course_participant.rb (that were initially not running) were fixed and made to run without errors.
• All tests in participant.rb, assignment_participant.rb and course_participant.rb- (except those related to publishing_rights due to absence of its functionality) ran successfully after changes were made to the three model files.

References

<references/>

See Also

• OSS - E808 project VCL link http://152.7.99.84:3000/ (may be disabled after a month)
• OSS - E808 project on GitHub https://github.com/npari/expertiza_oss808
• Expertiza project documentation can be found at http://wikis.lib.ncsu.edu/index.php/Expertiza
• The working Expertiza site is accessible at link http://expertiza.ncsu.edu/
• Expertiza on GitHub https://github.com/expertiza/expertiza