CSC/ECE 517 Fall 2015/oss E1557 GXM

E1557. Refactoring SignUpSheetController.rb and SignUpSheet.rb

In this project, we have unit tested various methods in SignUpSheetController.rb and SignUpSheet.rb as well as refactoring these methods. We first wrote unit tests to ensure we had adequate coverage for our methods. Then, we refactored our methods with confidence found from our unit tests. We found that directly testing the functions that we refactored provided better assurances that our refactorings were correct rather than manual testing the user interface.

Unit Testing

For our unit testing, we used the RSpec framework. We choose this framework since legacy tests of our methods were implemented with RSpec<ref>RSpec - http://rspec.info/</ref> . We also utilized the RSpec-Mocks<ref>RSpec-Mocks - https://github.com/rspec/rspec-mocks</ref> to test functions related to the database without having to use fixtures.

We wrote multiple controller tests and model tests for corresponding functions so as to test various scenarios and get 100% test coverage. We sought to obtain this high level of coverage to make refactoring more precise. We believe that highly tested code is resilient errors that could occur when refactoring.

Before we started working on Expertiza<ref>Expertiza - https://github.com/expertiza/expertiza</ref>, running RSpec on the codebase resulted in 992 / 4184 lines of code (23.71%) covered. After our work, running RSpec on the codebase resulted in 1144 / 4278 lines of code (26.74%) covered.

E.g. If you run the test in spec/controllers/sign_up_sheet_spec.rb by command "rspec spec/controllers/sign_up_sheet_spec.rb", then you can see the 100% line coverage for controller method save_topic_deadlines() in sign_up_sheet_controller.rb. For checking the coverage, open the index.html page coverage folder and go to respective controller.

Similar, run the tests in Model and Controllers folder to see 100% test coverage for following functions.

1. Create() in SignUpSheetController
2. self.add_signup_topic() in SignUpSheet
3. save_topic_deadlines() in SignUpSheetController
4. self.confirmTopic() in SignUpSheet

Refactoring

We performed various refactorings on our code to increase the codes readability and remove redundancy.

Extracting Variables

We extracted variable from complicated statements to increase the readability of the code. In the code base we were working with, there were a variety of lines of code that were more than 80 characters long. These lines of code were hard to read. So, we split them up into multiple lines of code by extracting statements found in function calls into their own variable.

Example:

duedate_subm = TopicDeadline.where(topic_id: topic.id, deadline_type_id: DeadlineType.find_by_name('submission').id, round: j).first

Became:

deadline_type_subm = DeadlineType.find_by_name('submission').id
duedate_subm = TopicDeadline.where(topic_id: topic.id, deadline_type_id: deadline_type_subm, round: j).first

Enforce Good Ruby Conventions

We attempted to enforce good Ruby conventions while refactoring the code. This entails doing things like correcting the formatting of the code to make it more readable. In addition to this, we converted a variety of iterators to follow good ruby conventions.

For example, sign_up_sheet.rb had the function self.add_sign_up_topic(assignment_id) which contained an iteration through a set of topics while manually maintaining an index variable. Manually maintaining the index variable can be problematic for future maintenance.

i=0
@topics.each { |topic|
  # Code not relevant to example omitted.
  i = i + 1
}

We replaced these with the Ruby each_with_index iterator. This allows us to pass the responsibility of maintaining the count of the index to the iterator.

@topics.each_with_index do |topic, i|

We replaced "=>" while passing parameter by colon (:)

So that

'due_at' => due_dates[session[:duedates][j]['id'].to_s 

became

due_at: due_dates[session[:duedates][j]['id'].to_s 

Creating New Methods

We have refactored the methods which were doing a lot of things into two or more methods so as to segregate the functionality and following the basic principal that one method should do only one thing. Also, this helped in making the methods more readable and easy to understand.

E.g. In sign_up_sheet.rb Model, "def sign_up_wailisted()" is the new method extracted from original method self.confirmTopic(). There are many more such examples which are done as part of refactoring.

How to Run Tests

1. Clone the repo
   

   git clone https://github.com/MaxSchweizer/expertiza

2. Change current directory to cloned repository
   
3. Run Command:
   

   rspec spec/controllers/sign_up_sheet_spec.rb

   rspec spec/models/sign_up_sheet_spec.rb

4. Check coverage:

   Open overage/index.html in browser and check line coverage for following methods

   1. Create() in sign_up_sheet_controller.rb in Controllers
      
   2. self.add_signup_topic() in sign_up_sheet.rb Models
      
   3. save_topic_deadlines() in sign_up_sheet_controller.rb in Controllers
      
   4. self.confirmTopic() in sign_up_sheet.rb in Models

Fixing Issue 580

We were not able to recreate the problem found in issue 580. After multiple attempts of trying to recreate the bug we began to examine the source code. We believe that someone else has already fixed the bug before we started working on the project.

References

<references></references>