Closed teodorciuraru closed 8 years ago
Teodor,
After analysing the code:
let matchingProperties: [AchievementProperty]? = properties.filter { propertyNames.contains($0.name!) }
);For example, if the AchievementsProperty is attached to a Achivement then the creation should be attached to it. You define and save(!) an property without it being attached to a achievement which could lead to bugs with misusage, for example, if you misspell the id which will impact negatively the app in production where you can't simply reset the device data. With that in mind there should be a method on Achievements called AddCondition/AddProperty that is responsible for appending a new condition to the existing ones assuring to one-to-many relation between Achievement and AchivementProperty. IMO, the id argument only adds unnecessary complexity.
I think you intended to move the Achivement and AchievementProperty to the achivementsManager to encapsulate. It is a good approach but you violated that encapsulation when you used that class on the ViewController.
With these thoughts in mind: You should delete those defineAchivement/defineAchivementProperty and create dedicated static methods on Achivement (static Create(...) -> Achivement) and AchivementProperty(static Create(...) -> AchivementProperty. However, leave the saving part to the achivementsManager.
Now for defining the achievement you would have to:
let achivement = Achivement.Create(...)
//add conditions
achivement.AddCondition(AchievementProperty.Create(...))
//receives, validates(maybe) and saves in the context
achivementsManager.defineAchivement(achievement)
Notice that with this approach you can test each object individually as the core objects.
Teodor, please resubmit the PR with a description on the changes you made and any feedback you think it is relevant.
Please provide further description regarding the current state and what is missing from the final PR.