CSC/ECE 517 Fall 2013/oss E811 syn

Refactor & test submitted_content_controller & submitted_content_helper

Expertiza is a web application that is used for Project/Assignment submissions. Students can form teams, select topics, submit assignments, review other's work and give feedback for reviews. Submission of work can be done in two ways:

• Hyperlinks
• Files/Folders

Submitted_Content_Controller and Submitted_Content_Helper are designed to handle operations on hyperlinks and files. They include methods to submit, verify and remove hyperlinks; and create, move, rename, submit and remove files.

Project Description

Classes: submitted_content_controller.rb (250 lines)
submitted_content_helper.rb (98 lines)
What they do: Allow users to submit files and links to Expertiza.
What needs to be done: The methods are long and complex, and contain a lot of duplicated code. In particular, there are two different ways of submitting files: as a homework submission, and uploading a review file requested by a review rubric. These should use common code as much as possible. The approach to deleting submitted links and submitted files should be analogous. There have been bugs in deleting hyperlinks, so be sure to test the code thoroughly.

Design Changes

• Code duplication<ref>http://en.wikipedia.org/wiki/Duplicate_code</ref> is generally considered a mark of poor or lazy programming style. Good coding style is generally associated with code reuse<ref>http://voices.yahoo.com/programming-code-reusability-11278802.html</ref>.

We identified a sequence of operations that are repetitively used in methods which are used to submit files for the first time and again in review. The code to check if a file exists or not and creating a new directory path for any new submission is common for both submission and review pages. Thus we separated it out as a different method ‘check_file_exists’ as shown below:

 def check_file_exists  curr_directory
    #check if file exists. If not, create a new file. Else, remove the existing file and then create new file.
    if !File.exists? curr_directory
      FileUtils.mkdir_p(curr_directory)
    else
      FileUtils.rm_rf(curr_directory)
      FileUtils.mkdir_p(curr_directory)
    end
  end

This method can now be called from both submit_file and custom_submit_file.

 def submit_file
    participant = AssignmentParticipant.find(params[:id])
    return unless current_user_id?(participant.user_id)

    file = params[:uploaded_file]
    participant.set_student_directory_num

    @current_folder = DisplayOption.new
    @current_folder.name = "/"
    if params[:current_folder]
      @current_folder.name = FileHelper::sanitize_folder(params[:current_folder][:name])
    end

    curr_directory = participant.get_path.to_s+@current_folder.name
    check_file_exists(curr_directory)

    safe_filename = file.original_filename.gsub(/\\/,"/")
    safe_filename = FileHelper::sanitize_filename(safe_filename) # new code to sanitize file path before upload*
    full_filename =  curr_directory + File.split(safe_filename).last.gsub(" ",'_') #safe_filename #curr_directory +
    File.open(full_filename, "wb") { |f| f.write(file.read) }

    if params['unzip']
      SubmittedContentHelper::unzip_file(full_filename, curr_directory, true) if get_file_type(safe_filename) == "zip"
    end
    participant.update_resubmit_times

    #send message to reviewers when submission has been updated
    participant.assignment.email(participant.id) rescue nil # If the user has no team: 1) there are no reviewers to notify; 2) calling email will throw an exception. So rescue and ignore it.

    redirect_to :action => 'edit', :id => participant.id
  end

• Long methods<ref>http://sourcemaking.com/refactoring/long-method</ref> are often caused due to unnecessary parameters. Long list of parameters reduce readability<ref>http://simpleprogrammer.com/2013/04/14/what-makes-code-readable-not-what-you-think/</ref> of code. Following changes are made to remove such unnecessary parameters:

Before Refactoring

Here the variable safename is declared and used only once. The assignment can be made inline by replacing it in the method call with actual logic in the below way:

After Refactoring

• Below is another name change from ‘fpath’ to ‘file_path’ which is made to eliminate the most common code smell<ref>http://www.codinghorror.com/blog/2006/05/code-smells.html</ref> ‘Excessively short identifiers’.

Before Refactoring

After Refactoring

• Consecutive assignments or modifications of the same variable can lead to “Long method” or “Excessive use of literals”. Such code smells are removed. For example, variable ‘newloc’ which is modified consecutively is refactored in the following way:

Before Refactoring:

  def move_selected_file
    old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
    new_loc = @participant.get_path 
    new_loc+= "/" 
    new_loc+= params[:faction][:move]
    begin
      FileHelper::move_file(old_filename, new_loc)
      flash[:note] = "The file was moved successfully from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
    rescue
      flash[:error] = "There was a problem moving the file: "+$!
    end
  end

After Refactoring:

  def move_selected_file
    old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
    new_location = @participant.get_path + "/" + params[:faction][:move]
    begin
      FileHelper::move_file(old_filename, new_location)
      flash[:note] = "The file was moved successfully from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
    rescue
      flash[:error] = "There was a problem moving the file: "+$!
    end
  end

Modified code reduces the method length and increases Readability.

• Unnecessary variable declarations which are never used in the method increase the length of code and confuse the readers. For example, the variable ‘display’ shown in the code snippet below was defined but never used in the method. Such variables can be removed.

Before Refactoring

The actual variable that was used which is ‘disp’ can now be renamed to ‘display’ which is more meaningful.

After Refactoring

• Many more minor code changes are made following the Ruby Coding guidelines. For example:

if !File.exist?(new_filename)

We should prefer to use 'unless' for such conditions:

unless File.exist?(new_filename)

List of Code Smells identified and removed

• Duplicated code: Identical or very similar code exists in more than one location. Code duplication frequently creates long, repeated sections of code that differ in only a few lines or characters. The length of such routines can make it difficult to quickly understand them. The repetition of largely identical code sections can conceal how they differ from one another, and therefore, what the specific purpose of each code section is. Often, the only difference is in a parameter value. The best practice in such cases is a reusable subroutine.
• Long method: A method, function, or procedure that has grown too large. Since the early days of programming people have realized that the longer a procedure is, the more difficult it is to understand. Older languages carried an overhead in subroutine calls, which deterred people from small methods. Modern OO languages have pretty much eliminated that overhead for in-process calls.
• Too many parameters: A long list of parameters in a procedure or function make readability and code quality worse. The more parameters a method has, the more complex it is. Limit the number of parameters you need in a given method, or use an object to combine the parameters.
• Excessively long identifiers: In particular, the use of naming conventions to provide disambiguation that should be implicit in the software architecture.
• Excessively short identifiers: The name of a variable should reflect its function unless the function is obvious. Pick a set of standard terminology and stick to it throughout your methods. For example, if you have Open(), you should probably have Close().
• Excessive use of literals: These should be coded as named constants, to improve readability and to avoid programming errors. Additionally, literals can and should be externalized into resource files/scripts where possible, to facilitate localization of software if it is intended to be deployed in different regions.
• Complex conditionals: Branches that check lots of unrelated conditions and edge cases that don't seem to capture the meaning of a block of code. Large conditional logic blocks, particularly blocks that tend to grow larger or change significantly over time have to be eliminated. Consider alternative object-oriented approaches such as decorator, strategy, or state.
• Dead Code<ref>http://en.wikipedia.org/wiki/Dead_code</ref>: Ruthlessly delete code that isn't being used. Unused code will result in Long classes or long methods.

Testing

To verify that the changes didn't break anything. One can login to the VCL instance using username/password: sample/sample. One can go to your work page in an assignment. Upload a hyperlink and delete it.

Future Work

• Polymorphism<ref>http://en.wikipedia.org/wiki/Polymorphism_(computer_science)</ref> can be implemented by pushing some methods to relevant classes. Following section of code contains certain method calls which are implemented in the same class. These can be abstracted into a different class for file operations:

  def file_folder_action
    @participant = AssignmentParticipant.find(params[:id])
    return unless current_user_id?(@participant.user_id)

    @current_folder = DisplayOption.new
    @current_folder.name = "/"
    if params[:current_folder]
      @current_folder.name = FileHelper::sanitize_folder(params[:current_folder][:name])
    end
    if params[:faction][:delete]        #call method to delete selected files
      delete_selected_files
    elsif params[:faction][:rename]     #call method to rename selected files
      rename_selected_file
    elsif params[:faction][:move]       #call method to move selected files
      move_selected_file
    elsif params[:faction][:copy]       #call method to copy selected files
      copy_selected_file
    elsif params[:faction][:create]     #call method to create new folder
      create_new_folder
    end

    redirect_to :action => 'edit', :id => @participant.id
  end

• Submit file functionality currently breaks with the latest expertiza master branch code which can be fixed by further exploration.
• Design changes made in partial files can be extended such that code reuse can be maximized.
• Many more code smells can be identified and removed to improve elegance.
• RSpec test cases for the changes made can be added.

Appendix: setup issues

Steps to setup the project:

• Extract source code in RubyMine using the following url: https://github.com/hsure/expertiza

(A) VCS -> Checkout from version control -> Github

(B) Give this url: https://github.com/hsure/expertiza -> Finish

• Confirm the sdk for RubyMine to ruby1.9.3 using File -> Settings
• Run bundle install
• Run - rake db:create:all
• Run - rake db:migrate
• From the Expertiza folder (project root) execute the command “rails server”

You should be able to view the test Database in MySQL.
Use the following credentials to explore the application:
Username: admin (Administrator)
Password: admin

Username: sample (User)
Password: sample

References

<references/>