Code style

Author: nygrenh

lib/background_course_refresher.rb

@@ -9,7 +9,7 @@ def do_refresh
         course.initial_refresh_ready = true
         course.save!
         Rails.logger.info "Finished background refresh on course id #{course.id}"
-      rescue => e
+      rescue StandardError => e
         Rails.logger.warn "Background refresh on course id #{course.id} failed!. Error #{e}"
       end
     end

lib/course_info.rb

@@ -13,9 +13,9 @@ def course_data(organization, course, opts = {})
     exercises = course.exercises.includes(:course, :available_points).to_a.natsort_by(&:name)
 
     @unlocked_exercises = course.unlocks
-      .where(user_id: @user.id)
-      .where(['valid_after IS NULL OR valid_after < ?', Time.now])
-      .pluck(:exercise_name)
+                                .where(user_id: @user.id)
+                                .where(['valid_after IS NULL OR valid_after < ?', Time.now])
+                                .pluck(:exercise_name)
 
     submissions_by_exercise = {}
     Submission.where(course_id: course.id, user_id: @user.id).each do |sub|
@@ -27,25 +27,25 @@ def course_data(organization, course, opts = {})
     end
 
     @course_list.course_data(organization, course, opts).merge(unlockables: course.unlockable_exercises_for(@user).map(&:name).natsort,
-                                                         exercises: exercises.map { |ex| exercise_data(ex) }.reject(&:nil?))
+                                                               exercises: exercises.map { |ex| exercise_data(ex) }.reject(&:nil?))
   end
 
   def course_data_core_api(course)
     UncomputedUnlock.resolve(course, @user)
     @unlocked_exercises = course.unlocks
-                              .where(user_id: @user.id)
-                              .where(['valid_after IS NULL OR valid_after < ?', Time.now])
-                              .pluck(:exercise_name)
+                                .where(user_id: @user.id)
+                                .where(['valid_after IS NULL OR valid_after < ?', Time.now])
+                                .pluck(:exercise_name)
 
     exercises = course.exercises.includes(:course, :available_points)
 
     unless @user.administrator? || @user.teacher?(course.organization) || @user.assistant?(course)
       exercises = exercises.where(hidden: false, disabled_status: 0)
       exercises = if @unlocked_exercises.empty?
-        exercises.where(unlock_spec: nil)
-      else
-        exercises.where(["unlock_spec IS NULL OR exercises.name IN (#{@unlocked_exercises.map {|_| '?'}.join(', ')})", *@unlocked_exercises])
-      end.select { |e| e._fast_visible_to?(@user)}
+                    exercises.where(unlock_spec: nil)
+                  else
+                    exercises.where(["unlock_spec IS NULL OR exercises.name IN (#{@unlocked_exercises.map { |_| '?' }.join(', ')})", *@unlocked_exercises])
+      end.select { |e| e._fast_visible_to?(@user) }
     end
 
     exercises = exercises.to_a.natsort_by(&:name)
@@ -94,7 +94,7 @@ def exercise_data(exercise)
       runtime_params: exercise.runtime_params_array,
       valgrind_strategy: exercise.valgrind_strategy,
       code_review_requests_enabled: exercise.code_review_requests_enabled?,
-      run_tests_locally_action_enabled: exercise.run_tests_locally_action_enabled?,
+      run_tests_locally_action_enabled: exercise.run_tests_locally_action_enabled?
     }
 
     data[:solution_zip_url] = @helpers.exercise_solution_zip_url(exercise) if @user.administrator?
@@ -112,25 +112,25 @@ def exercise_data_core_api(exercise)
     locked = exercise.requires_unlock? && !@unlocked_exercises.include?(exercise.name)
 
     data = {
-        id: exercise.id,
-        name: exercise.name,
-        locked: locked,
-        deadline_description: exercise.deadline_spec_obj.universal_description,
-        deadline: exercise.deadline_for(@user),
-        checksum: exercise.checksum,
-        return_url: @helpers.api_v8_core_exercise_submissions_url(exercise),
-        zip_url: @helpers.download_api_v8_core_exercise_url(exercise),
-        returnable: exercise.returnable?,
-        requires_review: exercise.requires_review?,
-        attempted: exercise.attempted_by?(@user),
-        completed: exercise.completed_by?(@user),
-        reviewed: exercise.reviewed_for?(@user),
-        all_review_points_given: exercise.all_review_points_given_for?(@user),
-        memory_limit: exercise.memory_limit,
-        runtime_params: exercise.runtime_params_array,
-        valgrind_strategy: exercise.valgrind_strategy,
-        code_review_requests_enabled: exercise.code_review_requests_enabled?,
-        run_tests_locally_action_enabled: exercise.run_tests_locally_action_enabled?,
+      id: exercise.id,
+      name: exercise.name,
+      locked: locked,
+      deadline_description: exercise.deadline_spec_obj.universal_description,
+      deadline: exercise.deadline_for(@user),
+      checksum: exercise.checksum,
+      return_url: @helpers.api_v8_core_exercise_submissions_url(exercise),
+      zip_url: @helpers.download_api_v8_core_exercise_url(exercise),
+      returnable: exercise.returnable?,
+      requires_review: exercise.requires_review?,
+      attempted: exercise.attempted_by?(@user),
+      completed: exercise.completed_by?(@user),
+      reviewed: exercise.reviewed_for?(@user),
+      all_review_points_given: exercise.all_review_points_given_for?(@user),
+      memory_limit: exercise.memory_limit,
+      runtime_params: exercise.runtime_params_array,
+      valgrind_strategy: exercise.valgrind_strategy,
+      code_review_requests_enabled: exercise.code_review_requests_enabled?,
+      run_tests_locally_action_enabled: exercise.run_tests_locally_action_enabled?
     }
 
     data[:solution_zip_url] = @helpers.download_api_v8_core_exercise_solution_url(exercise) if @user.administrator?
@@ -147,7 +147,7 @@ def sheets
   end
 
   def exercise_return_url(e)
-    "#{@helpers.exercise_submissions_url(e, format: 'json')}"
+    @helpers.exercise_submissions_url(e, format: 'json').to_s
   end
 
   def get_latest_submission(exercise)

lib/course_list.rb

@@ -5,15 +5,15 @@ def initialize(user, helpers)
     @helpers = helpers
   end
 
-  def course_list_data(organization, courses, opts={})
+  def course_list_data(organization, courses, opts = {})
     courses.map { |c| course_data(organization, c, opts) }
   end
 
   def course_list_data_no_organisation(courses, opts = {})
     courses.map { |c| course_data(c.organization, c, opts) }
   end
 
-  def course_data(organization, course, opts={})
+  def course_data(organization, course, opts = {})
     @course = course
     data = {
       id: course.id,
@@ -23,7 +23,7 @@ def course_data(organization, course, opts={})
       unlock_url: @helpers.organization_course_unlock_url(organization, course, format: :json),
       reviews_url: @helpers.organization_course_reviews_url(organization, course, format: :json),
       comet_url: CometServer.get.client_url,
-      spyware_urls: SiteSetting.value('spyware_servers'),
+      spyware_urls: SiteSetting.value('spyware_servers')
     }
 
     if opts[:include_points]
@@ -34,7 +34,7 @@ def course_data(organization, course, opts={})
             total_available: AvailablePoint.course_sheet_points(course, sheet)
           }
         end,
-        total_available: AvailablePoint.course_points_of_exercises(course, exercises),
+        total_available: AvailablePoint.course_points_of_exercises(course, exercises)
       }
     end
 
@@ -43,16 +43,17 @@ def course_data(organization, course, opts={})
         exercises.map do |ex|
           {
             name: ex.name,
-            conditions: JSON.parse(ex.unlock_spec),
+            conditions: JSON.parse(ex.unlock_spec)
           }
         end
       ]
     end
 
-      data
+    data
   end
 
   private
+
   def exercises
     @exercises ||= @course.exercises.select { |e| e.points_visible_to?(@user) }
   end

lib/course_refresher.rb

@@ -128,11 +128,11 @@ def refresh_course(course, options)
           @course.save!
           @course.exercises.each &:save!
 
-          CourseRefresher.simulate_failure! if ::Rails.env == 'test' && CourseRefresher.respond_to?('simulate_failure!')
+          CourseRefresher.simulate_failure! if ::Rails.env.test? && CourseRefresher.respond_to?('simulate_failure!')
         rescue StandardError, ScriptError # Some YAML parsers throw ScriptError on syntax errors
           @report.errors << $!.message + "\n" + $!.backtrace.join("\n")
           # Delete the new cache we were working on
-          FileUtils.rm_rf(@course.cache_path)                     unless options[:no_directory_changes]
+          FileUtils.rm_rf(@course.cache_path) unless options[:no_directory_changes]
           raise ActiveRecord::Rollback
         end
       end
@@ -143,18 +143,18 @@ def refresh_course(course, options)
       end
 
       course.reload # reload the record given as parameter
-      fail Failure.new(@report) unless @report.errors.empty?
+      raise Failure, @report unless @report.errors.empty?
       @report
     end
 
     def update_or_clone_repository
-      fail 'Source types other than git not yet implemented' if @course.source_backend != 'git'
+      raise 'Source types other than git not yet implemented' if @course.source_backend != 'git'
 
       if File.exist?("#{@old_cache_path}/clone/.git")
         begin
           # Try a fast path: copy old clone and git fetch new stuff
           copy_and_update_repository
-        rescue
+        rescue StandardError
           FileUtils.rm_rf(@course.clone_path)
           clone_repository
         end
@@ -164,7 +164,7 @@ def update_or_clone_repository
     end
 
     def copy_and_update_repository
-      FileUtils.cp_r("#{@old_cache_path}/clone", "#{@course.clone_path}")
+      FileUtils.cp_r("#{@old_cache_path}/clone", @course.clone_path.to_s)
       Dir.chdir(@course.clone_path) do
         sh!('git', 'remote', 'set-url', 'origin', @course.source_url)
         sh!('git', 'fetch', 'origin')
@@ -184,7 +184,7 @@ def check_directory_names
       Find.find(@course.clone_path) do |path|
         relpath = path[@course.clone_path.length..-1]
         if File.directory?(path) && exdirs.any? { |exdir| exdir.start_with?(path) } && relpath.include?('-')
-          fail "The directory #{path} contains a dash (-). Currently that is forbidden. Sorry."
+          raise "The directory #{path} contains a dash (-). Currently that is forbidden. Sorry."
         end
       end
     end
@@ -248,9 +248,7 @@ def update_exercise_options
 
           e.options = metadata
 
-          if (e.new_record? && e.course.refreshed?)
-            e.disabled!
-          end
+          e.disabled! if e.new_record? && e.course.refreshed?
 
           e.save!
         rescue SyntaxError
@@ -290,23 +288,22 @@ def update_available_points(no_directory_changes = false)
         clone_path = Pathname("#{@course.clone_path}/#{exercise.relative_path}")
 
         points_data = points_for(exercise, no_directory_changes)
-        if points_data[0].is_a? Hash
-          point_names += points_data.map { |x| x[:points] }.flatten
-        else
-          point_names += points_data.flatten
-        end
+        point_names += if points_data[0].is_a? Hash
+                         points_data.map { |x| x[:points] }.flatten
+                       else
+                         points_data.flatten
+                       end
 
         point_names += review_points
 
         added = []
         removed = []
 
         point_names.each do |name|
-          if exercise.available_points.none? { |point| point.name == name }
-            added << name
-            point = AvailablePoint.create(name: name, exercise: exercise)
-            exercise.available_points << point
-          end
+          next unless exercise.available_points.none? { |point| point.name == name }
+          added << name
+          point = AvailablePoint.create(name: name, exercise: exercise)
+          exercise.available_points << point
         end
 
         exercise.available_points.to_a.clone.each do |point|
@@ -334,19 +331,19 @@ def get_c_exercise_points(exercise)
       TestScannerCache.get_or_update(@course, exercise.name, hash) do
         all_points = Set.new
         Dir.chdir(full_path) do
-          sh!(%w(make test))
-          sh!(%w(make get-points > points.txt), {escape: false})
+          sh!(%w[make test])
+          sh!(%w[make get-points > points.txt], escape: false)
 
           tmc_available_points = File.join(full_path, 'test', 'tmc_available_points.txt')
-          if File.exists? tmc_available_points
+          if File.exist? tmc_available_points
             IO.readlines(tmc_available_points).map(&:strip).each do |line|
               if line =~ /\[.*\] \[.*\] (.*)/
-                $1.split(' ').map(&:strip).each { |p| all_points << p}
+                Regexp.last_match(1).split(' ').map(&:strip).each { |p| all_points << p }
               else
                 raise "Warning: weird line in available points file: #{line}"
               end
             end
-          elsif File.exists?(File.join(full_path, 'points.txt'))
+          elsif File.exist?(File.join(full_path, 'points.txt'))
             available_points_content = IO.readlines("#{full_path}/points.txt")
             # drop makefile output
             # This is how initial check tests used to work.
@@ -360,8 +357,8 @@ def get_c_exercise_points(exercise)
             raise "Could not extract points for makefile exercise: #{exercise}"
           end
 
-          sh!(%w(make clean))
-          FileUtils.rm("#{full_path}/points.txt") if File.exists?(File.join(full_path, 'points.txt'))
+          sh!(%w[make clean])
+          FileUtils.rm("#{full_path}/points.txt") if File.exist?(File.join(full_path, 'points.txt'))
         end
         all_points
       end
@@ -461,12 +458,12 @@ def set_permissions
       parent_dirs = Course.cache_root.sub(::Rails.root.to_s, '').split('/').reject(&:blank?)
       (0..(parent_dirs.length)).each do |i|
         dir = "#{::Rails.root}/#{parent_dirs[0..i].join('/')}"
-        sh!('chmod', chmod, dir) unless chmod.blank?
-        sh!('chgrp', chgrp, dir) unless chgrp.blank?
+        sh!('chmod', chmod, dir) if chmod.present?
+        sh!('chgrp', chgrp, dir) if chgrp.present?
       end
 
-      sh!('chmod', '-R', chmod, @course.cache_path) unless chmod.blank?
-      sh!('chgrp', '-R', chgrp, @course.cache_path) unless chgrp.blank?
+      sh!('chmod', '-R', chmod, @course.cache_path) if chmod.present?
+      sh!('chgrp', '-R', chgrp, @course.cache_path) if chgrp.present?
     end
 
     def invalidate_unlocks

lib/course_refresher/block_comment_based_filter.rb

@@ -3,7 +3,7 @@
 class CourseRefresher
   class BlockCommentBasedFilter # Abstract
     def applies_to?(_file_path)
-      fail 'abstract method'
+      raise 'abstract method'
     end
 
     def filter_for_stub(text)
@@ -52,8 +52,8 @@ def remove_solution_blocks(text)
 
     def uncomment_stubs(text)
       text.gsub(stub_regexp) do
-        before = $1
-        after = $2
+        before = Regexp.last_match(1)
+        after = Regexp.last_match(2)
         before + after
       end
     end
@@ -82,11 +82,11 @@ def solution_file_regexp
     end
 
     def comment_begin
-      fail 'abstract method'
+      raise 'abstract method'
     end
 
     def comment_end
-      fail 'abstract method'
+      raise 'abstract method'
     end
 
     def resc(s)

lib/course_refresher/java_filter.rb

@@ -9,20 +9,12 @@ def applies_to?(file_path)
 
     def filter_for_stub(text)
       text = super(text)
-      if text
-        remove_html_comments(text)
-      else
-        nil
-      end
+      remove_html_comments(text) if text
     end
 
     def filter_for_solution(text)
       text = super(text)
-      if text
-        remove_html_comments(text)
-      else
-        nil
-      end
+      remove_html_comments(text) if text
     end
 
     def remove_html_comments(text)