View source for E1920 Fix Code Climate Issues

This wiki page describes the changes are made in the Expertiza source code to implement OSS Project "E1920 - Fix Code Climate issues in models with names beginning with ''Se'' thru ''Z''" for the CSC/ECE 517: "Object-Oriented Design and Development", Spring semester of the 2019.

== Introduction ==

This Wiki page is created per CSC/ECE 517: "Object-Oriented Design and Development", OSS Project 3 requirements (Spring, 2019).

Our [http://wiki.expertiza.ncsu.edu/index.php?title=E1920_Fix_Code_Climate_Issues#Team_Contacts_and_Contribution team] was assigned to [https://docs.google.com/document/d/17tkcuA7AWgGEzDGpjEJ7ktPzbSr-D89MJZd-cXzSCuE/edit#heading=h.jes8j9o3codt "E1920 - Fix Code Climate issues in models with names beginning with ''Se'' thru ''Z''"] OSS Project. In this page we describe what work has been completed by our team to improve overall quality, readability, maintainability and efficiency of the Expertiza source code. What approaches we used to resolve issues reported by Code Climate, what are the remaining issues in the Expertiza code, our suggestions for further improvements and our testing strategy used to ensure integrity of the code. Each section below provides clear and descriptive review on the subject we cover.

We start with introduction to Expertiza application and Code Climate tool. How Code Climate is used to improve Expertiza source code.
=== Expertiza ===

[https://expertiza.ncsu.edu/ Expertiza] is a peer to peer application whereby students gain knowledge through submitting their work into the system and review the work of others. This is system diverges from the customary systems where a instructor or body of teaching staff would grade the papers/assignments and provide feedback to the students. This process misses out on a learning opportunity for students performing reviews. Allowing students to see the work of others allows them to see more code examples and from different aspects. This exercise during the review process forces the student to think through lecture material and apply during the process. The Expertiza application can also be used as an iterative process from class to class. The examples of the current semester can be used by students of subsequent semesters to identify shortcomings and suggest further improvements.

=== Code Climate ===

Code Climate is an analysis software used to improve the quality and security of software. The Code Climate generated results for the Expertiza project can be found [https://codeclimate.com/github/expertiza/expertiza/code here].

Code Climate utilizes various engines to analyze the code to look for issues that may concern potential security issues, (.i.e. marking code to html_safe), or add to maintainability issues (.i.e number of lines in method exceeding 25 lines). Some of the engines include: RuboCop, Brakeman, Bundler-Audit, [https://docs.codeclimate.com/docs/list-of-engines etc.]

Code Climate can be ran from either a 3-rd party remote server of via the local application using CLI. The Expertiza project uses the 3-rd party remote server to analyze the code. In order to use Code Climate via remote server, desired repository must be registered with Code Climate. Following [https://docs.codeclimate.com/docs this] process allows to test the code for quality issues after each git commit/push to a registered repository.

== Project Description ==

Some of the Expertiza source code violates many of the [https://rubyonrails.org/ Ruby on Rails] best practices and must be rectified to improve code readability, robustness, efficiency, and maintainability of the Expertiza application.

Our team was assigned to fix ''all'' code smells detected by [https://codeclimate.com/github/expertiza/expertiza/code Code Climate] in the given set of Expertiza source code files. In this section we specify what files we have changed to improve overall software of the Expertiza project. We also list and describe the issues that we were asked to ignore during implementation of our project.

=== Files Involved ===

All files in the [https://github.com/expertiza/expertiza/tree/master/app/models app/models] directory with names beginning with '''Se''' (e.g. [https://github.com/expertiza/expertiza/blob/master/app/models/section_header.rb section_header.rb]) through '''Z''', except [https://github.com/expertiza/expertiza/blob/master/app/controllers/users_controller.rb users_controller.rb] file were targeted by our team to be Code Climate issue free files. Therefore, our team was assigned exactly with 50 (fifty) files to fix in total. Some of the files had no issues reported by the Code Climate to begin with, and hence were not modified. However, changes/improvements performed on other files had an affect globally on the files outside of the app/models scope and also had to be modified to adapt changes we implemented. For example, renaming method ''is_leaf'' as follows:
  def leaf?
in the [https://github.com/expertiza/expertiza/blob/master/app/models/team_user_node.rb team_user_node.rb] file require to change all callers to this method in different files accordingly. Such changes had to be done in the following files (for this particular example):
<ul>
<li> models/assignment_node.rb
<li> models/node.rb
<li> models/questionnaire_node.rb
<li> views/tree_display/_row_header.html.erb
</ul>

These files are outside of the requirements specifications, but we had to modify them in order to maintain correct behavior of the App. There were few such cases when we had to modify multiple files to fix one or more Code Climate issue(s). The list of all of files changed can be found in the Pull Request discussed in this article.

=== Ignored Code Climate Smells ===

There are some Code Climate issues that were ignored and not resolved intentionally.

The following issues are exempt based the given requirements specifications and were not fixed:
<ul>
<li> Assignment Branch Condition size for [method name] is too high.
<li> Perceived complexity for [method name] is too high.
<li> Cyclomatic complexity for [method name] is too high.
<li> Method [method name] has a Cognitive Complexity of ''XX'' (exceeds 5 allowed). Consider refactoring.
<li> File [file name] has ''XXX''  lines of code (exceeds 250 allowed). Consider refactoring.
<li> Class [class name] has ''XX'' methods (exceeds 20 allowed). Consider refactoring.
<li> Method [method name] has ''XX'' lines of code (exceeds 25 allowed). Consider refactoring.
</ul>

There are also two Code Climate issues that our team decided to ignore after consulting our mentor and discussing their impact on the existing code and Expertiza project. It was determined that the following Code Climate issues reported as False Positive and may not need to be resolved:
<ul>
<li> Mass assignment is not restricted using attr_accessible.
<li> Potentially dangerous attribute available for mass assignment.
</ul>

Finally, there was a duplicate code issue that was ignored due to a future project. The following files have a duplicate method, self.export_fields(options):
<ul>
<li> user.rb
<li> participant.rb
</ul>

For further clarification with regard to ignored Code Climate issues please contact our mentor.

== Project Implementation ==

In this section we talk about implementation of our project. We start with the most common issues reported by Code Climate and what approach we used to resolve them. We provide some code samples taken from the files and show their improvements. These changes/improvements are not trivial and require understanding of the code. Therefore, we provide implementation details of such issues along with detailed explanations.

Furthermore, we list all of the issues reported by Code Climate that we fixed. These changes/improvements are trivial and require no explanation. Therefore, any implementation details are excluded for these type of issues.

Next, we cover section with additional improvements that we have done along with fixes for Code Climate issues. These improvements do not directly relate to Code Climate issues; however, they improve overall quality of the code, its readability, maintenance and efficiency.

We also list and talk about remaining issues reported by Code Climate that were intentionally ignored and left in the project. We provide further suggestions based on our experience to the Expertiza software developers.

Finally, we provide out pull request that our team created as we started to work on this project.

=== Most Common Issues Reported by Code Climate ===

As we worked on the Code Climate issues for the Expertiza project, we noticed a set of common code smells and issues made by software developers. Since these issues can easily be avoided and most importantly, avoiding them improves code quality, we decided to ''warn'' all Expertiza developers about them by providing the list of these issues, their descriptions, how they can be avoided, and how the code quality can be improved.

* Do not use ''Time.now'' or ''Time.parse'' without zone.
This is very common issue reported by Code Climate. Since time is relative and may have different values (UTC, EST, and etc), it is a good practice to specify the time zone, whenever the ''Time'' class used.

'''Note''' that ''Time.parse'' and ''Time.zone.parse'' behave differently for invalid or unknown time. While  returns an error for invalid or unknown times,  returns nil for those types.
<ul>
<ul>
<li> ''Time.parse'' - returns an error for invalid or unknown time. Generally, can be handled by ''rescue''
<li> ''Time.zone.parse'' - returns ''nil'' for invalid or unknown time. It must be handled differently.
</ul>
</ul>
Therefore, always handle ''nil'' appropriately if invalid or unknown time is given (with ''if/else'' block, ''return'', or ''or'' statements, and etc.) Do not assume that time will always be valid instance. For example, (code sample is used from ''app/models/student_task.rb'' file):
  # Bad - use Time without zone and rescue if time is invalid or unknown 
  stage_deadline: (Time.parse(participant.stage_deadline) rescue Time.now + 1.year)
  
  # Good - use Time with zone and using or statement to handle nil case
  stage_deadline: (Time.zone.parse(participant.stage_deadline) || (Time.zone.now + 1.year))
Above example is good practice how ''Time'' class should be used.
* Method ''x'' has ''n'' arguments (exceeds 4 allowed). Consider refactoring:
Any method that takes more than 4 arguments must be refactored with accordance to good RoR coding standards. There are multiple ways to refactor signature of such methods and there is no single standard way of resolving this issue. Each method must be considered separately and requires unique approach to refactor, utilizing hash, array and other Ruby's built-in libraries and tools. Below we provide one sample of how fixed such issue utilizing Ruby's built-in hash (code sample is used from ''app/models/vm_question_response_row.rb'' file):
  # Bad - initialize method has 5 arguments
  def initialize(question_text, question_id, weight, question_max_score, seq)
    @question_text = question_text
    @weight = weight
    @question_id = question_id
    @question_seq = seq
    @question_max_score = question_max_score
  
  # Good - initialize method has 1 argument, hash
  def initialize(question_data)
    @question_text = question_data[:text]
    @question_id = question_data[:id]
    @weight = question_data[:weight]
    @question_max_score = question_data[:max_score]
    @question_seq = question_data[:seq]
''Note'' that since method signature is changed, it of the callers must be changed accordingly to adapt new method signature. For example, the following is illustration of such change to adapt above refactored method signature (code sample is used from ''app/models/vm_question_response.rb'' file):
  # Bad - passing in 5 arguments to the constructor
  row = VmQuestionResponseRow.new(question.txt, question.id, question.weight, question_max_score, question.seq)
  
  # Good - passing in 1 argument to the constructor, hash
  row = VmQuestionResponseRow.new(text: question.txt, id: question.id, weight: question.weight, max_score: question_max_score, seq: question.seq)
* Prefer ''each'' over ''for'':
We noticed there is a lot of iteration in the code using ''for'' loop instead of ''each''. Generally, you want to avoid any ''for'' loops and use ''each'', ''each_index'', ''each_with_index'', ''upto'' and etc. For example, (code sample is used from ''app/models/sign_up_sheet.rb'' file):
  # Bad - use of for loop
  for user_signup_topic in user_signup
    return false if user_signup_topic.is_waitlisted == false
  
  # Good - use of each
  user_signup.each do |user_signup_topic|
    return false if user_signup_topic.is_waitlisted == false
Similarly, if you need to iterate the given block, passing in integer as an index, utilize ''each_index'', ''each_with_index'', ''upto'' instead of ''for''. For example, (code sample is used from ''app/models/sign_up_sheet.rb'' file):
  # Bad - use of for loop
  for round in 1..@review_rounds
    process_review_round(assignment_id, due_date, round, topic)
  
  # Good - use of upto
  1.upto(@review_rounds) do |r|
    process_review_round(assignment_id, due_date, r, topic)
* Specify an '':inverse_of option'':
Whenever there are two models defined with many|one-to-one relationship (''has_many''|''has_one'' and ''belongs_to'' association) out of scope of each other or with certain options used (e.g. ''foreign_key:'' and etc.), Active Record is unable to determine the inverse association of them. This would cause redundant look up operation performed on the database for the same object. It is not really an issue that could cause significant problems, but for such massive application like Experiza, it could affect performance and cause delay in response. Therefore, the '':inverse_of'' option must be specified manually to use the associated object in memory instead of loading it from database twice. For example, (code sample is used from ''app/models/teams_user.rb'' and ''app/models/team_user_node.rb'' files respectively):
  class TeamsUser < ActiveRecord::Base
    has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy, inverse_of: 'node_object'
  end
  
  class TeamUserNode < Node
    belongs_to :node_object, class_name: 'TeamsUser', inverse_of: :team_user_node
  end
* Specify a '':dependent'' option:
The [https://guides.rubyonrails.org/association_basics.html#the-has-many-association has_many] association should always be specified with one of the [https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_many-label-Options :dependent] options, which specifies the behavior of the associated objects whenever their owner is destroyed. This is common practice and should be utilized adequately for the Expertiza app to work correctly. No specifying '':dependent'' option could lead to run time errors, unpredictable behavior of the app, and creating orphan objects that will never be referenced again. Specify a '':dependent'' option whenever utilizing ''has_many'' association. For example, (code sample is used from ''app/models/site_controller.rb'' file):
  # Bad - not specifying a :dependent option
  class SiteController < ActiveRecord::Base
    has_many :controller_actions
  
  # Good - specifying a :dependent option
  class SiteController < ActiveRecord::Base
    has_many :controller_actions, dependent: :destroy
* Tagging a string as html safe may be a security risk. This Code Climate issue warns the user that strings tagged with html_safe can leave your site vulnerable to XSS attacks. To overcome this issue, we chose to use content_tag and tag helpers to create HTML tags. By using this practice we are able to create HTML strings that are actually HTML safe and we minimize the possibility of XSS attacks. A common use is:
  # Bad - uses html_safe
  html = '<b style="color: #986633;">' + self.txt + '</b>'.html_safe

  # Good - use content_tag
  html = content_tag(:b, self.txt, {style: "color: #986633"}, false)
We did find that there was some HTML code that was not properly rendered using the content_tag paradigm. This HTML code involved user supplied answers. These answers were created using a rich text editor called TinyMCE. TinyMCE wraps parts of the text in HTML tags and our mentor warned us against trying to decode these strings. For these strings we used the raw() function to allow them to properly render. For example,
  concat self.txt + raw(answer.comments)
This use of raw() is in two files, text_field.rb and text_area.rb. If a better solution is found, please let the authors know.
* Use a guard clause instead of wrapping the code inside a conditional expression.
This is very common issue reported by Code Climate that can be easily avoided by properly utilizing `if` statements applying some logic. In general, avoid using bulk of ''if/else'' statements whenever possible. For example, (code sample is used from ''app/models/sign_up_topic.rb'' file):
  # Bad - too many conditional expressions that also contain nested ones
  if !no_of_students_who_selected_the_topic.nil?
    if topic.max_choosers > no_of_students_who_selected_the_topic.size
      return true
    else
      return false
    end
  else
    return true
  end
  
  # Good - use guard close
  return true if no_of_students_who_selected_the_topic.nil?
  topic.max_choosers > no_of_students_who_selected_the_topic.size
Above is typical example when 9 lines of code (LoC) can be reduced to 2 LoC and overall code quality improved. 
* Use ''find_by'' instead of ''where.first'':
Always use ''find_by'' with an attempt to find the first record matching the specified conditions.

'''Note''' that ''find_by'' and ''where.first'' behave differently if no record was found based on specified conditions.
<ul>
<ul>
<li> ''Model.find_by'' - if there is a record match, returns the object. If there is no record match, returns ''nil''
<li> ''Model.where.first'' - if there is a record match, returns first ActiveRecord::Relation matched. If there is no record match, returns an ''empty'' ActiveRecord::Relation
</ul>
</ul>
Therefore, always handle ''nil'' appropriately if there is no record match (with ''if/else'' block, ''return'' statement and etc.) Do not assume that the record will always be found (since ''empty'' relation skips iteration and ''nil'' throws an error if attempt to iterate). For example,
  user = User.find_by(name: row_hash[index.to_sym].to_s)
  raise ImportError, "The user, " + row_hash[index.to_sym].to_s.strip + ", was not found." if user.nil?
Similar to this issue Code Climate reported "Use ''find_by'' instead of dynamic ''find_by_name''" issue. It is resolved in the same manner as the above issue by changing ''find_by_name'' to ''find_by'' query and properly handling returned edge-cases (e.g. ''nil''). 
* Use ''snake_case'' for method names and for variable names:
There are a lot of instances where developers use ''camelCase'' instead of ''snake_case'' in the Expertiza project naming methods and variables. Since Expertiza project is based on RoR, it is  preferred to use ''snake_case'' for all method and variable names. For example, (code sample is used from ''app/models/sign_up_sheet.rb'' and ''app/models/sign_up_topic.rb'' files receptively):
  # Bad method name - camelCase
  def self.slotAvailable?(topic_id)
  
  # Good method name - snake_case
  def self.slot_available?(topic_id)
  
  # Bad variable name - camelCase
  dropDate = AssignmentDueDate.where(parent_id: assignment.id, deadline_type_id: '6').first
  
  # Good method name - snake_case
  drop_date = AssignmentDueDate.where(parent_id: assignment.id, deadline_type_id: '6').first

=== Resolved Code Climate Issues ===

Along with [http://wiki.expertiza.ncsu.edu/index.php/E1920_Fix_Code_Climate_Issues#Most_Common_Issues_Reported_by_Code_Climate Most Common Issues Reported by Code Climate] we also fixed and verified numerous other code smells. The following is the list of all issues we have resolved. We do not provide solutions for them since most of them were trivial code changes.

<ul>
<li> Argument ''x'' was shadowed by a local variable before it was used.
<li> Avoid more than 3 levels of block nesting.
<li> Avoid too many return statements within the method.
<li> Avoid using ''update_attribute'' because it skips validations.
<li> Combine ''_type" type="text">'' and " into a single string literal, rather than using implicit string concatenation.
<li> Consider simplifying this complex logical expression.
<li> Convert ''if'' nested inside ''else'' to ''elsif''.
<li> Do not introduce global variables.
<li> Do not place comments on the same line as the ''end'' keyword.
<li> Do not prefix reader method names with ''get_''.
<li> Do not prefix writer method names with ''set_''.
<li> Do not use ''DateTime.parse.strftime'' without zone. Use one of ''Time.zone.parse.strftime'', ''DateTime.current'', ''DateTime.parse.strftime.in_time_zone'', ''DateTime.parse.strftime.utc'', ''DateTime.parse.strftime.getlocal'', ''DateTime.parse.strftime.iso8601'', ''DateTime.parse.strftime.jisx0301'', ''DateTime.parse.strftime.rfc3339'', ''DateTime.parse.strftime.to_i'', ''DateTime.parse.strftime.to_f'' instead.
<li> Extra empty line detected at method body beginning.
<li> Extra empty line detected at method body end.
<li> ''end'' at ''xxx'', ''xx'' is not aligned with ''unless'' (or ''def'') at ''xxx'', ''xx''.
<li> Identical blocks of code found in ''n'' locations. Consider refactoring. 
<li> Line is too long.
<li> Method ''x'' is defined at both ''app/models/y.rb'' and ''app/models/z.rb''
<li> Optional arguments should appear at the end of the argument list.
<li> Place the condition on the same line as ''elsif''.
<li> Prefer Date or Time over DateTime.
<li> Redundant use of ''Object#to_s'' in interpolation.
<li> Rename ''has_x'' to ''x?''.
<li> Rename ''is_x'' to ''x?''.
<li> Replace class var ''@@variable_name'' with a class instance var.
<li> Similar blocks of code found in ''n'' locations. Consider refactoring.
<li> Unnecessary spacing detected.
<li> Unused method argument - ''x''. If it's necessary, use ''_'' or ''_x'' as an argument name to indicate that it won't be used.
<li> Use ''find_by'' instead of ''where.first''.
<li> Use ''x.zero?'' instead of ''x == 0''.
<li> Use ''=='' if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
<li> Useless assignment to variable - ''x''.
<li> Variable ''x'' used in void context.
</ul>

=== Additional Improvements ===

While our primary task was to fix code smells reported by Code Climate, we also attempted to improve overall quality of code along the way. We introduced several minor changes that we believe improve the code, and encourage all software engineers working on the Expertiza source code to maintain clean and efficient software. All additional improvements are listed and described below.

* Meaningful names for class and instance variables.
There were several files where names for class or instance variables did not meet good naming conventions. For example, in the ''app/models/vm_question_response_row.rb'' file, instance variable 
  no_of_columns
indicating number of columns that refer to count of reviews, was re-named with
  count_columns
Above is only an example of how we attempted to improve readability of the code.
* More comments where needed.
In general there is sufficient amount of comments to fully understand the code and follow the flow. However, some of the functionalities are not as trivial and additional comments help a lot faster understand what the code does.
We added such comments in all the places where we fixed Climate Code issues. We also ensured to provide only sufficient comments to keep code as clean as possible and at the same time to clarify any complexity.
* Eliminate DRY code.
We tried to combine all of the duplicated code into a single functionality and eliminate any repetitive statements. Most of these issues were taken care by fixing "Similar blocks of code found in ''n'' locations" and "Identical blocks of code found in ''n'' locations" Code Climate issues. However, in some files Code Climate did not report such issues, although there were obvious repetitive statements. For example, ''add_reviews'' method in the ''app/models/vm_question_response.rb'' file had 4 places of the repetitive code and it was different only by data retrieved from various tables as follows:
  if @questionnaire_type == 'ReviewQuestionnaire'
    reviews = ...
    reviews.each do |review|
      review_mapping = ReviewResponseMap.find_by(id: review.map_id)
      # Repetitive code as everywhere else
  elsif @questionnaire_type == "AuthorFeedbackQuestionnaire"
    reviews = participant.feedback
    reviews.each do |review|
      review_mapping = FeedbackResponseMap.find_by(id: review.map_id)
      # Repetitive code as everywhere else
  elsif @questionnaire_type == "TeammateReviewQuestionnaire"
    reviews = participant.teammate_reviews
    reviews.each do |review|
      review_mapping = TeammateReviewResponseMap.find_by(id: review.map_id)
      # Repetitive code as everywhere else
  elsif @questionnaire_type == "MetareviewQuestionnaire"
    reviews = participant.metareviews
    reviews.each do |review|
      review_mapping = MetareviewResponseMap.find_by(id: review.map_id)
      # Repetitive code as everywhere else
The above duplicate code was refactored to eliminate all repetitive statements and make the code more efficient as follows:
  if @questionnaire_type == 'ReviewQuestionnaire'
    reviews = ...
    response_map = 'ReviewResponseMap'.constantize
  elsif @questionnaire_type == 'AuthorFeedbackQuestionnaire'
    reviews = participant.feedback
    response_map = 'FeedbackResponseMap'.constantize
  elsif @questionnaire_type == 'TeammateReviewQuestionnaire'
    reviews = participant.teammate_reviews
    response_map = 'TeammateReviewResponseMap'.constantize
  elsif @questionnaire_type == 'MetareviewQuestionnaire'
    reviews = participant.metareviews
    response_map = 'MetareviewResponseMap'.constantize
  end
  # All repetitive code was extracted and now is common
  unless reviews.nil?
    reviews.each do |review|
      review_mapping = response_map.find_by(id: review.map_id)
      # Repetitive code
Again this is only single sample where improvements was made.

=== Remaining Issues ===

Unfortunately, Expertiza has some remaining issues reported by Code Climate in the files under ''app/models'' directory (names beginning with '''Se''' through '''Z''') that we were asked to ignore in this project. These issues described in the [http://wiki.expertiza.ncsu.edu/index.php?title=E1920_Fix_Code_Climate_Issues#Ignored_Code_Climate_Smells Ignored Code Climate Smells] section above and why there were ignored. Although these code smells do not impose any problems into existing code and application, we strongly believe that all of remaining issues must be fixed.

There is also an issue with methods that were commented out in the original code. While the team removed code that we changed, we also made a conscious decision to leave existing commented out methods in the code and let the original maintainer decide if the comment should be removed. The following files had existing commented out methods:
# sign_up_topic (2)
# simicheck_webservice.rb (1)

We did not find any further issues besides what we already reported and fixed it.

=== Further Suggestions ===

We suggest and encourage to fix all of the remaining issues that were intentionally ignored per the projects requirements in all of the files under ''app/models'' directory.


We also suggest to maintain clean, efficient, well-commented, and well-tested code in the Expertiza project. Prior delivering any changes to the ''master'' or ''beta'' branches, we recommend to test your software in the following manner:
# Code inspection
# Passing existing Rspec tests
# Adding new Rspec test that cover changes
# Running through a live demo in the Expertiza instance through the UI
Furthermore, we noticed that currently Expertiza project has ~53% test coverage, which is relatively small test coverage for such a massive project. Remember, "Debugging - sucks, and Testing - rocks!". We encourage Expertiza software developers to reach at least (minimum) 85% test coverage. That must cover all vital functionalities of the application and reduce any possibilities of introducing new issues into the system.

=== Pull Request ===
We created a [https://github.com/expertiza/expertiza/pull/1368 Pull Request] as soon as we had at least one fix on our [https://github.com/gshugh/expertiza/tree/beta forked beta branch].

GitHub pull request is an integral part of solving Code Climate issues and delivering fixes into the Expertiza beta branch. We verified all our Code Climate fixes on the local working repositories, which we registered with Code Climate tool. Every new git commit/push was triggering new Code Climate job and we could verify if our changes fixed the issues. A typical workflow was as follows:
* Code Climate issues were resolved and tested locally
* Changes were pushed to local repositories registered with Code Climate
* New git commit/push was triggering new Code Climate verification job
* Verified that resolved issues were not reported on the latest Code Climate job
All our changes are visible and can be tracked in the single pull requests we created as soon as we had one fix.

== Testing ==
To test integrity of the code and ensure that our changes/improvements did not introduce new issues/errors, we followed four simple test steps:
# Code inspection
# Passing/fixing existing RSpec tests
# '''Developing new RSpec tests'''
# Running through a live demo in the Expertiza instance through the UI
''Note'' that per project's requirement specifications we were not required to develop and write newly RSpec tests. However, ultimately we strive to achieve crucial improvements over code quality, robustness, and maintainability for the files that we have worked on. We believe we achieved our goal. We did not do this to gain extra credit or something similar. We did it because we believe in testing and convinced that nothing can improve Expertiza source code like testing.
 
There are 2 types of frameworks that we used to verify our code modifications/creation within the models: RSpec and Travis CI.

RSpec was used to locally test the changes we had implemented without having to Push the change to the GitHub, this is discussed more in-depth in the section below.

Travis CI is a more comprehensive testing used by the Expetiza team to verify code changes from multiple sources, (.i.e forks from students). Travis CI analyzes the code once the changes are Pushed to the GitHub server for desired repository. Travis CI needs to be setup independently for desired repository.
=== Existing Rspec Testing ===

To verify that the code changes we made did not break the existing application and its functionalities, we used the RSpec Testing Framework to test all the code.

Some of the models already had existing RSpecs tests created prior we started on work on the project. For those models, we ran the corresponding test via the terminal window. For example, to test the simi_check_webservice model:
   rspec spec/models/simi_check_webservie_spec.rb

This executes the RSpec test for the simi_check_webservice model, app/models/simi_check_webervice.rb. As you can see, there is a particular format that is used when naming the RSpec file:
    model_name_spec.rb

Once the execution of the RSpec model testing is completed, results are displayed:
    $ rspec spec/models/simi_check_webservice_spec.rb
    [Coveralls] Set up the SimpleCov formatter.
    [Coveralls] Using SimpleCov's 'rails' settings.
    
    Randomized with seed 29018
    .........
    
    Finished in 1.63 seconds (files took 28.35 seconds to load)
    10 examples, 0 failures, 10 pending
    
    Randomized with seed 29018
    
    Coverage report generated for RSpec to /home/expertiza_developer/expertiza/coverage. 1192 / 17393 LOC (6.85%) covered.
    [Coveralls] Outside the CI environment, not sending data.

In addition to running the individual RSpec tests, we also ensured that all previous RSpec tests passed. To achieve this, we executed the following command in the terminal window:
    $ bundle exec rspec spec/controllers spec/models

We ran this command against master branch as a baseline, which produced the following results:

    817 examples, 0 failure, 16 pending

These tests represented 1510 lines of code and 58.54% code coverage for our models.

However, while working with the existing RSpec tests, we noticed that some models have relatively low coverage (> 40%). Therefore, we could not rely strictly on existing RSpec tests and guarantee correctness of our code changes by running them. We decided to write additional Rspec tests that directly test all our code changes and add them into existing spec files by modifying existing tests and boost the test coverage on files that we modified. For example, we fixed multiple Code Climate issues in the [https://github.com/gshugh/expertiza/commit/b9aac55b6eb331c1db295b7e7dd45fbbdba19abc app/models/team.rb] file. Specifically, complications of such issues as "Use a guard clause instead of wrapping the code inside a conditional expression", "Use ''find_by'' instead of ''where.first''", and others were resolved in ''import'', ''export'', and ''handle_duplicate'' method calls. Most of the changes are listed below:

[[File: app_models_teams_1.png]]
[[File: app_models_teams_2.png]]

Any of the above changes may break existing functionality of the code without proper testing. Moreover, we noticed that existing ''spec/models/team_spec.rb'' Rspec file does not thoroughly tests these method calls and had very a little to none of such tests. Hence, in order to verify and test all our changes, we wrote additional Rspec tests and added them into ''spec/models/team_spec.rb'' that specifically tests our code changes. ''Note'' that in the following test cases, we cover all possible scenarios, including edge-case scenarios. Check out [https://github.com/gshugh/expertiza/commit/96cec7fa82c1c55e31e0196d3a025111a4fe134a spec/models/team_spec.rb] for full list of Rspec tests. The following is a list of Rspec we developed to test all our changes in the ''app/models/team.rb'' file:

[[File: rspec_models_team_spec_1.png]]
[[File: rspec_models_team_spec_2.png]]
[[File: rspec_models_team_spec_3.png]]

''Note'' that this is only one small example we show here of additional Rspec tests we developed. Full list of newly developed and added Rspec tests can be found in our Git Pull Request. Also note that by adding new spec test cases, we increased test code coverage on all of the models. The test coverage for the ''app/models/team.rb'' model was increased up to 88.51% as follows:

[[File: spec_models_team_spec_test_coverage.png]]

=== New RSpec Testing ===

Moreover, while working with the existing Rspec tests, besides low coverage on some of the tests, we also noticed that some models are tested indirectly via other Rspec tests. Meaning there is no dedicated spec test file for some of the models that we fixed. Since we were not aware of what spec files are responsible for testing some models that have no dedicated spec test file, we did not know what are the exact set of test cases existing Rspec testing framework runs on these file (e.g. edge-case scenarios and etc). Similarly, as with existing Rspec tests we found out that existing specs would not suffice for the changes that we made. Therefore, we created '''new''' dedicated Rspec test files and wrote test cases that test our changes directly.

For example, we fixed multiple Code Climate issues in the ''app/models/teams_user.rb'' file. At the moment, there is no corresponding Rspec file for that model in the system. Therefore, we created a new [https://github.com/gshugh/expertiza/blob/beta/spec/models/teams_user_spec.rb spec/models/teams_user_spec.rb] file to test directly our code changes and boost the code coverage on that model. The following is the change that we did in the ''app/models/teams_user.rb'' file:

[[File: app_models_teams_user_changes.png]]

The following is newly created corresponding ''spec/models/teams_test_spec.rb'' test file included in our Git Pull Request having all general-purpose tests in it as follows:

[[File: spec_models_teams_user_1.png]]
[[File: spec_models_teams_user_2.png]]
[[File: spec_models_teams_user_3.png]]
 
''Note'' that by creating new corresponding spec test for the ''app/models/teams_user.rb'' model, we achieved 100% test coverage on that model, whereas before it was around 60.53%.

[[File: spec_models_teams_user_test_coverage.png]]

Again this is only one example we show here of the files that we created. More additional spec files were created to test our code changes. For example, we made extensive changes by fixing "Tagging a string as html safe may be a security risk" Code Climate issue. Most of those models did not have corresponding Rspec files, so we added those RSpec files to allow developers to have some confidence in the code before deployment. The test coverage on those models was also increased.

After all of these changes to the RSpec files, our beta branch produced the following results:

    886 examples, 0 failure, 16 pending

This represented 1440 lines of code and 68.19% of code coverage for the models of interest. The number of lines of code decreased by 4.64% (from 1510 to 1440), an increase in code coverage of 16.48% (from 58.54% to 68.19%) and an increase in test examples of 8.45% (from 817 to 886). The increase in the percentage of code coverage was due to a combination of the decrease in the number of lines and the increase in the number of test examples.

We also compared the test coverage before and after implementation of our project and there was increase of 1% on models with names starting with ''Se'' through ''Z''. To compare the results on the files that were fixed we ran the following command before and after our project implementation:

  rspec spec/models/[se-z]*.rb

The following are results before our project implementation:

[[File: se_z_test_coverage_before.png]]

And the following are results after the project implementation:

[[File: se_z_test_coverage_after.png]]

Although 1% does not seem to be big improvements. However, this is 1% of the total code coverage and for such massive project 1% test coverage increase is significant!

=== UI Testing ===

We concentrated our UI testing based on how changes to Code Climate issues affected the interface between callers and methods. Two main culprits were found.
# Change of ''Time.parse'' to ''Time.zone.parse''. While ''Time.parse'' returns an error for invalid or unknown times, ''Time.zone.parse'' returns ''nil'' for those types.
# Change of ''where().first'' to ''find_by()'' query. Using ''where().first'' returns empty records if record is not found, while ''find_by()'' returns ''nil'' if there was no match.
Note both of the above changes are well described in the [http://wiki.expertiza.ncsu.edu/index.php?title=E1920_Fix_Code_Climate_Issues#Most_Common_Issues_Reported_by_Code_Climate Most Common Issues Reported by Code Climate] section above. The section describes how such cases must be handled properly.

The user interface testing concentrated on four types of user stories.
# General functionality, could users still access the basic menu items from before.
# Test time functionality, such as sorting based on timestamp.
# Test searching functionality (involving all changes of ''where().first'' to ''find_by()'').
# Pages that previously used html_safe
UI testing helped to reveal all the problems that were introduced along with our fixes and running through UI live demos ensured that we have fully functioning and properly working Expertiza app.

== Conclusion ==

Code Climate is a powerful tool that finds flaws in the source code of the Expertiza project. We would like to acknowledge that all issues reported by Code Climate on the files beginning with ''Se'' thru ''Z'' under ''app/models'' directory were fixed (except [http://wiki.expertiza.ncsu.edu/index.php?title=E1920_Fix_Code_Climate_Issues#Ignored_Code_Climate_Smells Ignored Code Climate Smells])! There are no more Code Climate issues found!

Although, in our project we were not asked to write additional Rspec tests, we dedicated some of our time to do so. The goal of writing additional spec test cases was to validate our code changes, make sure our changes did not cause any problems in the app, and boost the test coverage on the models we worked on. We achieved all our goals and tested all our changes with the help of Rspec tests that we wrote. We also ensured that our additional spec tests boost the code coverage.

We encourage software developers working on the Expertiza project to maintain clean, readable, well-commented and efficient code. We suggest to achieve at least 85% of test coverage on all of the existing files by writing more Rspec tests and considering more possible scenarios (i.e, edge-case scenarios).

We would like to thank our mentor (Zachariah Mathews) and teaching staff for all the assistance and help they provided to us during the project implementation.

== Team Contacts and Contribution ==

We have 3 (three) members in our team and 1 (one) mentor was assigned to us. For any project-related questions/concerns please contact us:

<ul>
<li> Christopher Adkins - cladkins@ncsu.edu
<li> George Hugh - gshugh@ncsu.edu
<li> Nikolay G. Titov - ngtitov@ncu.edu
<li> Zachariah Mathews (mentor) - zmathew@ncsu.edu
</ul>

If you have questions or concerns related to a specific file that was improved/modified by our team, please contact an engineer who worked on the file based on the [https://docs.google.com/spreadsheets/d/1bX2XizitGGoajFT6Br_y6MQjAwBCsX5WCVlx3xg8Jp8/edit?ts=5c87a06c#gid=0 Assignment Grid] we created to keep track of our progress and contribution. Please CC in email all team members and our mentor.

==References==
* Ruby [https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/InverseOf]
* CodeClimate [https://docs.codeclimate.com/docs]
* Expertiza [https://github.com/expertiza/expertiza#expertiza]
* E. F. Gehringer, L. M. Ehresman, S. G. Conger and P. A. Wagle, "Work in Progress: Reusable Learning Objects Through Peer Review: The Expertiza Approach," Proceedings. Frontiers in Education. 36th Annual Conference, San Diego, CA, 2006, pp. 1-2. [http://ieeexplore.ieee.org.prox.lib.ncsu.edu/stamp/stamp.jsp?tp=&arnumber=4117076&isnumber=4116830]
* The ''has_many'' Association [https://guides.rubyonrails.org/association_basics.html#the-has-many-association]
* Module ActiveRecord::Associations::ClassMethods Options [https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_many-label-Options]
* TimeZone Class [https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/TimeZone]