Extract diff logic to its own class

lib/brakeman.rb

@@ -313,38 +313,14 @@ module Brakeman
   # Compare JSON ouptut from a previous scan and return the diff of the two scans
   def self.compare options
     require 'json'
+    require 'brakeman/differ'
     raise ArgumentError.new("Comparison file doesn't exist") unless File.exists? options[:previous_results_json]
 
-    previous_results = JSON::load(File::open(options[:previous_results_json]))['warnings']
+    previous_results = JSON.parse(File::open(options[:previous_results_json]).read, :symbolize_names =>true)[:warnings]
 
     tracker = run(options)
-    new_results = JSON.parse(tracker.report.to_json)['warnings']
-
-    warnings = {}
-    warnings[:new] = new_results - previous_results
-    warnings[:fixed] = previous_results - new_results
-
-    # second pass to cleanup any vulns which have changed in line number only
-    warnings[:new].each_with_index do |new_warning, new_warning_id|
-      warnings[:fixed].each_with_index do |fixed_warning, fixed_warning_id|
-        if matches_except_line new_warning, fixed_warning
-          warnings[:new].delete_at new_warning_id
-          warnings[:fixed].delete_at fixed_warning_id
-        end
-      end
-    end
-
-    warnings
-  end
-
-  private
+    new_results = JSON.parse(tracker.report.to_json, :symbolize_names =>true)[:warnings]
 
-  def self.matches_except_line new_vuln, fixed_vuln
-    new_vuln.keys.reject{|k,v| k == 'line'}.each do |attr|
-      if new_vuln[attr] != fixed_vuln[attr]
-        return false 
-      end
-    end
-    true
+    Brakeman::Differ.new(new_results, previous_results).diff
   end
 end

lib/brakeman/checks.rb

 require 'thread'
+require 'brakeman/differ'
 
 #Collects up results from running different checks.
 #
@@ -64,22 +65,7 @@ class Brakeman::Checks
   def diff other_checks
     my_warnings = self.all_warnings
     other_warnings = other_checks.all_warnings
-
-    diff = {}
-
-    diff[:fixed] = other_warnings - my_warnings
-    diff[:new] = my_warnings - other_warnings
-
-    # second pass to cleanup any vulns which have changed in line number only
-    diff[:new].each_with_index do |new_warning, new_warning_id|
-      diff[:fixed].each_with_index do |fixed_warning, fixed_warning_id|
-        if new_warning.matches_except_line fixed_warning
-          diff[:new].delete_at new_warning_id
-          diff[:fixed].delete_at fixed_warning_id
-        end
-      end
-    end
-    diff
+    Brakeman::Differ.new(my_warnings, other_warnings).diff
   end
 
   #Return an array of all warnings found.

lib/brakeman/differ.rb

+# extracting the diff logic to it's own class for consistency currently handles
+# an array of Brakeman::Warnings or plain hash representations.  
+class Brakeman::Differ
+  DEFAULT_HASH = {:new => [], :fixed => []}
+  attr_reader :old_warnings, :new_warnings
+
+  def initialize new_warnings, old_warnings
+    @new_warnings = new_warnings
+    @old_warnings = old_warnings
+  end
+
+  def diff
+    # get the type of elements
+    return DEFAULT_HASH if @old_warnings.empty? && @new_warnings.empty?
+
+    warnings = {}
+    warnings[:new] = @new_warnings - @old_warnings
+    warnings[:fixed] = @old_warnings - @new_warnings
+
+    second_pass(warnings)
+  end
+
+  # second pass to cleanup any vulns which have changed in line number only
+  # Horrible O(n^2) performance.  Keep n small :-/
+  def second_pass(warnings)
+    warnings[:new].each_with_index do |new_warning, new_warning_id|
+      warnings[:fixed].each_with_index do |fixed_warning, fixed_warning_id|
+        if matches_except_line new_warning, fixed_warning
+          warnings[:new].delete_at new_warning_id
+          warnings[:fixed].delete_at fixed_warning_id
+        end
+      end
+    end
+
+    warnings
+  end
+
+  def matches_except_line new_warning, fixed_warning
+    # can't do this ahead of time, as callers may be expecting a Brakeman::Warning
+    if new_warning.is_a? Brakeman::Warning 
+      new_warning = new_warning.to_hash
+      fixed_warning = fixed_warning.to_hash
+    end
+
+    new_warning.keys.reject{|k,v| k == :line}.each do |attr|
+      if new_warning[attr] != fixed_warning[attr]
+        return false 
+      end
+    end
+    true
+  end
+end

lib/brakeman/warning.rb

@@ -152,15 +152,4 @@ class Brakeman::Warning
 
     JSON.dump self.to_hash
   end
-
-  def matches_except_line other_warning
-    other_warning_hash = other_warning.to_hash
-    self_hash = self.to_hash
-    self_hash.keys.reject{|k,v| k == :line}.each do |attr|
-      if self_hash[attr] != other_warning_hash[attr]
-        return false 
-      end
-    end
-    true
-  end
 end