Object counts per condition context #199
Conversation
There was a problem hiding this comment.
As this is getting more complex than the initial implementation, maybe it makes sense to add respective unit tests. What do you think?
That would provide the opportunity to target edge cases specifically and verify that they work. Do you think it would be much effort creating tests?
| .getChildren(groupDef); | ||
| if (children != null && children.iterator().hasNext()) { | ||
| for (EntityDefinition def : children) { | ||
| if (groupDef instanceof TypeEntityDefinition) |
| increase(groupDef, 1); | ||
| } | ||
| else { | ||
| ChildContext context = path.get(0); |
There was a problem hiding this comment.
Isn't here always the last path element relevant? Because that should be the path element that the child has in addition to its parent. The parent is on the instance side represented by the group.
There was a problem hiding this comment.
Hm, I am starting to see that you changed the way that addToPopulation works by specifying what you want to limit the count to, via the provided EntityDefinition. It would be good if you would reflect that in the method documentation. "the group entity definition" does not tell anything - my first assumption was that it is directly related to the group.
|
|
||
| for (Object value : values) { | ||
| if (value instanceof Group) { | ||
| addToPopulation((Group) value, propertyDef); | ||
| addToPopulation((Group) value, groupDef, subPath); |
There was a problem hiding this comment.
Here you pass all Group values on, but shouldn't that be done only for those that were counted for this level?
For instance: If it did not match the condition at this level, it should not be counted on the next.
| Bundle-Version: 3.1.0.qualifier | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-1.7 | ||
| Bundle-Vendor: wetransform GmbH | ||
| Import-Package: eu.esdihumboldt.hale.common.align.model, |
There was a problem hiding this comment.
By convention these projects/bundles should rather be placed in common instead of ui and named accordingly, as they don't have any UI dependencies.
There was a problem hiding this comment.
Ok, I thought as it contains helper class for ui service so named with UI. I'll change according to your suggestion.
related to issue #174