six2six / fixture-factory

Generator fake objects from a template
Apache License 2.0
445 stars 88 forks source link

fix #44: override property definition on `gimme` call #47

Closed jonathanhds closed 10 years ago

aparra commented 10 years ago

Hello Jonathan, thanks for the pull request. I did the review of your code. Please refactor these points:

1) This code snippet is repeated in ObjectFactory class (old problem): Rule rule = templateHolder.getRules().get(label); if (rule == null) { throw new IllegalArgumentException(String.format(NO_SUCH_LABEL_MESSAGE, templateHolder.getClazz().getName(), label)); }

Please, refactor the snippet in a private method, this way you will avoid public method call public method in the same class: public T gimme(String label) { return gimme(label, new Rule()); } Call the gimme method without overwriting rules should not be a call gimme with empty rule, the key idea is create an object with the found rule. You will take a better gimme method with the creation (refactoring) of a private method to find the rule.

2) Probably, with the first refactoring this will not be necessary anymore. "Fica a dica :)". ObjectFactory#gimme has a reallocation: rule = new Rule(rule, propertiesToOverride); [line 73]. Refactor to ... this.createObject(new Rule(rule, propertiesToOverride)); Less is more.

3) import java.util.*; import a lot of classes, import only the necessary.

Let me know if I was clear.

jonathanhds commented 10 years ago

Hello Ander,

thanks for your feedback. This is my first time contributing for this project. I hope to help a lot from now on!

I've fixed the code according with your suggestions. I've also added the capacity to create a list of objects with a Rule to override the properties.

Let me know if this changes comply with the project's needs!

aparra commented 10 years ago

Ok. I'll close the pull request. I'm waiting the changes. Please create a new pull request. Thanks a lot.