CSC/ECE 517 Fall 2013/oss E811 syn

Introduction

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 is generally considered a mark of poor or lazy programming style. Good coding style is generally associated with code reuse.

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

• 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

• Long methods are often caused due to unnecessary parameters. Long list of parameters reduce readability 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 ‘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.

• 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)

Appendix: setup issues

Git repository: https://github.com/hsure/expertiza Application URL: 152.1.13.219::3000

Steps to setup the project: 1. 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 2. Confirm the sdk for RubyMine to ruby1.9.3 using File -> Settings 3. Run bundle install 4. Run - rake db:create:all 5. Run - rake db:migrate 6. From the Expertiza folder (project root) execute the command “rails server”

You should be able to view the test Database in MySQL in the following way:

Use the following credentials to explore the application: Username: admin (Administrator) Password: admin

Username: sample (User) Password: sample

In order to test, login with user credentials and navigate to Assignments page. In Homework1 select ‘Your work’ and submit a hyperlink.

Code Changes: 1.

2.

List of Code Smells identified and removed: • Duplicated code: Identical or very similar code exists in more than one location. • Long method: A method, function, or procedure that has grown too large. • Too many parameters: A long list of parameters in a procedure or function make readability and code quality worse. • 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. • 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.