ValueObject should not be duplicated #87

Merged
merged 3 commits into from May 17, 2012

Projects

None yet

2 participants

@senny
Contributor
senny commented May 1, 2012

This is a small addition to make Virtus::ValueObjects #dup and #clone methods return self. Since ValueObjects can't be mutated having multiple instances representing the same value makes no sense.
This is only a simple implementations and there are cases when we could reuse existing objects. I think it makes sense though to have these specs with a simple implementation.

The topic was discussed here: #62
The issue can be closed after merging this pull-request.

I also took the freedom to add /bin to the .gitignore file to ignore bundlers generated binstubs. Feel free to dump this commit if you don't agree ;)

@dkubb dkubb commented on an outdated diff May 17, 2012
spec/integration/default_values_spec.rb
@@ -41,4 +49,12 @@ def default_editor_title
subject.editor_title.should == 'UNPUBLISHED: Top Secret'
end
+ context 'with a ValueObject' do
+ it 'should not duplicate the ValueObject' do
+ page1 = Examples::Page.new
+ page2 = Examples::Page.new
+ page1.reference.object_id.should == page2.reference.object_id
dkubb
dkubb May 17, 2012 Collaborator

@senny you can probably change this to page1.reference.should equal(page2.reference)

@dkubb dkubb commented on an outdated diff May 17, 2012
...rtus/value_object/instance_methods/duplicates_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe Virtus::ValueObject::InstanceMethods, 'duplication' do
+ let(:described_class) do
+ Class.new do
+ include Virtus::ValueObject
+
+ attribute :name, String
+ end
+ end
+
+ subject { described_class.new }
+
+ it '#clone returns the same instance' do
+ subject.object_id.should == subject.clone.object_id
dkubb
dkubb May 17, 2012 Collaborator

I would change this to subject.should equal(subject.clone)

@dkubb dkubb commented on an outdated diff May 17, 2012
...rtus/value_object/instance_methods/duplicates_spec.rb
+ let(:described_class) do
+ Class.new do
+ include Virtus::ValueObject
+
+ attribute :name, String
+ end
+ end
+
+ subject { described_class.new }
+
+ it '#clone returns the same instance' do
+ subject.object_id.should == subject.clone.object_id
+ end
+
+ it '#dup returns the same instance' do
+ subject.object_id.should == subject.dup.object_id
dkubb
dkubb May 17, 2012 Collaborator

Same as above ;)

@dkubb dkubb commented on an outdated diff May 17, 2012
lib/virtus/value_object.rb
@@ -53,6 +53,24 @@ def initialize(attributes = {})
def with(attribute_updates)
self.class.new(get_attributes(&FILTER_NONE).merge(attribute_updates))
end
+
+ # ValueObjects are immutable and can't be cloned. They always represent the same value.
+ #
+ # @return [self]
+ #
+ # @api public
+ def clone
+ self
+ end
+
+ # ValueObjects are immutable and can't be duped. They always represent the same value.
dkubb
dkubb May 17, 2012 Collaborator

What do you think about using alias dup clone instead of declaring the same message body twice?

@dkubb dkubb commented on the diff May 17, 2012
...rtus/value_object/instance_methods/duplicates_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe Virtus::ValueObject::InstanceMethods, 'duplication' do
+ let(:described_class) do
+ Class.new do
+ include Virtus::ValueObject
+
+ attribute :name, String
+ end
+ end
+
+ subject { described_class.new }
dkubb
dkubb May 17, 2012 Collaborator

The default for subject is described_class.new so this probably isn't necessary.

senny
senny May 17, 2012 Contributor

actually the implicit subject is not exactly described_class.new. I tried it but I guess since i override the described_class method it gets confused and tries to instantiate a Virtus::ValueObject::InstanceMethods object.

dkubb
dkubb May 17, 2012 Collaborator

Oh, you're right. I missed the part where you were overriding described_class above, focusing only on the subject part.

Collaborator
dkubb commented May 17, 2012

@senny I added a few comments to this PR. I agree with the overall premise and would be happy to merge it once the suggested changes are made (assuming @solnic doesn't object).

Contributor
senny commented May 17, 2012

@dkubb thanks for the review. Applied most suggested changes and updated the PR.

@dkubb dkubb merged commit 85741a3 into solnic:master May 17, 2012
Collaborator
dkubb commented May 17, 2012

@senny thanks for the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment