Make delete_if/reject faster and fix other mutators

[#1559 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>

2 files changed, 61 insertions, 13 deletions

diff --git a/activesupport/lib/active_support/ordered_hash.rb b/activesupport/lib/active_support/ordered_hash.rb
index fa68db5604..3def0be639 100644
--- a/activesupport/lib/active_support/ordered_hash.rb
+++ b/activesupport/lib/active_support/ordered_hash.rb
@@ -18,19 +18,13 @@ module ActiveSupport
       end
 
       def delete(key)
-        array_index = has_key?(key) && index(key)
-        if array_index
-          @keys.delete_at(array_index)
+        if has_key? key
+          index = @keys.index(key)
+          @keys.delete_at index
         end
         super
       end
 
-      def delete_if
-        super
-        sync_keys!
-        self
-      end
-
       def reject!
         super
         sync_keys!
@@ -41,9 +35,6 @@ module ActiveSupport
         dup.reject!(&block)
       end
 
-      alias_method :super_keys, :keys
-      private :super_keys
-
       def keys
         @keys
       end
@@ -68,10 +59,30 @@ module ActiveSupport
         keys.each {|key| yield [key, self[key]]}
       end
 
+      alias_method :each_pair, :each
+
+      def clear
+        super
+        @keys.clear
+        self
+      end
+
+      def shift
+        k = @keys.first
+        v = delete(k)
+        [k, v]
+      end
+
+      def merge(other_hash)
+        result = dup
+        other_hash.each {|k,v| result[k]=v}
+        result
+      end
+
       private
 
       def sync_keys!
-        (@keys - super_keys).each { |k| @keys.delete(k) }
+        @keys.delete_if {|k| !has_key?(k)}
       end
     end
   end
diff --git a/activesupport/test/ordered_hash_test.rb b/activesupport/test/ordered_hash_test.rb
index ca5fce6267..0e2aa4543d 100644
--- a/activesupport/test/ordered_hash_test.rb
+++ b/activesupport/test/ordered_hash_test.rb
@@ -36,9 +36,11 @@ class OrderedHashTest < Test::Unit::TestCase
 
     @ordered_hash[key] = value
     assert_equal @keys.length + 1, @ordered_hash.length
+    assert_equal @ordered_hash.keys.length, @ordered_hash.length
 
     assert_equal value, @ordered_hash.delete(key)
     assert_equal @keys.length, @ordered_hash.length
+    assert_equal @ordered_hash.keys.length, @ordered_hash.length
 
     assert_nil @ordered_hash.delete(bad_key)
   end
@@ -84,6 +86,17 @@ class OrderedHashTest < Test::Unit::TestCase
     @ordered_hash.each_with_index { |pair, index| assert_equal [@keys[index], @values[index]], pair}
   end
 
+  def test_each_pair
+    values = []
+    keys = []
+    @ordered_hash.each_pair do |key, value|
+      keys << key
+      values << value
+    end
+    assert_equal @values, values
+    assert_equal @keys, keys
+  end
+
   def test_delete_if
     (copy = @ordered_hash.dup).delete('pink')
     assert_equal copy, @ordered_hash.delete_if { |k, _| k == 'pink' }
@@ -103,4 +116,28 @@ class OrderedHashTest < Test::Unit::TestCase
     assert_equal copy, @ordered_hash
     assert !new_ordered_hash.keys.include?('pink')
   end
+
+  def test_clear
+    @ordered_hash.clear
+    assert_equal [], @ordered_hash.keys
+  end
+
+  def test_merge
+    other_hash =  ActiveSupport::OrderedHash.new
+    other_hash['purple'] = '800080'
+    other_hash['violet'] = 'ee82ee'
+    merged = @ordered_hash.merge other_hash
+    assert_equal merged.length, @ordered_hash.length + other_hash.length
+    assert_equal @keys + ['purple', 'violet'], merged.keys
+
+    @ordered_hash.merge! other_hash
+    assert_equal @ordered_hash, merged
+    assert_equal @ordered_hash.keys, merged.keys
+  end
+
+  def test_shift
+    pair = @ordered_hash.shift
+    assert_equal [@keys.first, @values.first], pair
+    assert !@ordered_hash.keys.include?(pair.first)
+  end
 end
\ No newline at end of file