Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a cache for factories requested in Fixture.from static method. #63

Closed
wants to merge 1 commit into from

Conversation

quikkoo
Copy link

@quikkoo quikkoo commented Sep 30, 2014

A ObjectFactory operates like a singleton for each class when was created in Fixture.from static method. Example:

Entity e1 = Fixture.from(Entity.class).gimme("valid");
Entity e2 = Fixture.from(Entity.class).gimme("valid");

This snippet call a Fixture.from method twice and create two ObjectFactory for two diferent fixtures from Entity. Two identical factory was created under the same TemplateHolder object provided by templates map in Fixture class. Even with no expansive computations theres no need to create a ObjectFactory in each call of Fixture.from. With this modification, a single ObjectFactory is created and two different fixtures for the Entity class.

@nykolaslima
Copy link
Member

Looks good to me. What do you think @aparra @ahirata?

@ahirata
Copy link
Member

ahirata commented Oct 1, 2014

This cache may cause unexpected behavior to occur because, currently, ObjectFactory has a Processor as mutable state.

Suppose we have the following code in testA:
Fixture.from(Entity.class).uses(someProcessor).gimme("label");

And in testB we have:
Fixture.from(Entity.class).gimme("label");

In this case, someProcessor may be called in testB depending on whether testA ran before testB or not.

@nykolaslima
Copy link
Member

@ahirata you are right. I had forgot about processors.

@quikkoo do you understand? We can't use the same instance, because if user choose to use Processor for one object, the other objects of this class will always use the first declared Processor.

@quikkoo
Copy link
Author

quikkoo commented Oct 1, 2014

I undestand and agree, i never had to use a Processor before, so i didn't saw this issue.

But, in this case, the uses function causes a side effect over an instance of ObjectFactory and, the factory behaves as a builder and not as a factory.

IMHO, if a factory creates objects in different ways, it should have different methods for this, such as gimme overloads, or with different factories. So, i suggest change the function uses to create a new ObjectFactory instance instead of changing the internal state of current object.

public ObjectFactory uses(Processor processor) {
    return new ObjectFactory(templateHolder, processor);
}

Thus, the factories for each class can be shared in all application, and take a step to be immutable object in a near future, with a final modifier in your fields.

Obs.: For this change, all tests in ObjectFactoryWithProcessorTest will fail and need to be rewritten.

@nykolaslima
Copy link
Member

I don't see advantages in doing this. I believe that ObjectFactory is actually a Builder, you are building the Factory to build your object.

What do you think @ahirata?

@ahirata
Copy link
Member

ahirata commented Oct 3, 2014

Misleading names aside, considering that your initial pull request was concerned about having 1 ObjectFactory instance for each gimme call I believe this other suggestion is less than ideal from the user's perspective.

Since it's backwards compatible, if people don't change their code, they would build, not 1, but 2 instances. However, if they do change all their tests (in order to share the instance), I guess they would gain very little by skipping the instantiation because, as you said, there are no expensive computations in the constructor.

But don't get me wrong, just like my previous comment, I'm just trying to explain the effects of this change and why it's not very efficient, in my opinion. I'm not saying that there isn't design issues inside fixture-factory, it's just that this change alone doesn't really benefit the user nor does it create enough value to the framework's codebase so that the trade-off would worth it.

As a side note, I thought it was a little bit odd that tests failed so I did some checking and it turns out that they are fine. The actual reason for them to fail is because there's a bug in ObjectFactory's constructor. I'm gonna fix that. Thanks for pointing it out!

@ahirata ahirata closed this Oct 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants