Skip to content

Improve value_object's #hash uniformity #233

Merged
merged 1 commit into from Dec 19, 2013

4 participants

@codesnik

Array#hash is designed to be uniform, unlike reduce(:^).hash

class Point
  include Virtus.value_object
  values do
     attribute :x, Integer
     attribute :y, Integer
  end
end

Point.new(x: 1, y: 1).hash == Point.new.hash # was true
Point.new(x: 1).hash == Point.new(y: 1).hash # was true
@codesnik codesnik Improve value_object's #hash uniformity
Array#hash is designed to be uniform, unlike reduce(:^).hash

class Point
  include Virtus.value_object
  values do
     attribute :x, Integer
     attribute :y, Integer
  end
end

Point.new(x: 1, y: 1).hash == Point.new.hash # was true
Point.new(x: 1).hash == Point.new(y: 1).hash # was true
28c718e
@coveralls

Coverage Status

Coverage decreased (-0.02%) when pulling 28c718e on codesnik:fixing_hash into 6e7f6df on solnic:master.

@dkubb dkubb commented on the diff Dec 19, 2013
lib/virtus/support/equalizer.rb
@@ -65,7 +65,7 @@ def define_cmp_method
def define_hash_method
keys = @keys
define_method(:hash) do
- keys.map { |key| send(key).hash }.reduce(self.class.hash, :^)
+ keys.map { |key| send(key) }.push(self.class).hash
@dkubb
Collaborator
dkubb added a note Dec 19, 2013

In similar code, I've done something like the following because I wanted the hash keys to be factored into the hash:

keys.map { |key| key.hash ^ send(key).hash }.reduce(self.class.hash, :^)

I like your solution though because the work to create the Array has already been done anyway. Might as well call Array#hash rather than using Enumerable#reduce to xor the hashes.. it's simpler and probably optimized.

@codesnik
codesnik added a note Dec 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dkubb
Collaborator
dkubb commented Dec 19, 2013

I just applied this fix to similar code within equalizer, eg: https://github.com/dkubb/equalizer/blob/master/lib/equalizer.rb#L66-L71

@solnic
Owner
solnic commented Dec 19, 2013

Thanks, I'm gonna merge it in even though there's no test covering this as it's a deprecated module anyway and I'll remove it in 2.0.0. Once new equalizer is released I will bump its dependency here and release new version of virtus.

@solnic solnic merged commit 7ad62e2 into solnic:master Dec 19, 2013

1 check passed

Details default The Travis CI build passed
@dkubb dkubb referenced this pull request in dkubb/equalizer Dec 23, 2013
Merged

Bump version to 0.0.9 #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.