Bugfix/fix diamond problem & refactoring #1590#1607
Bugfix/fix diamond problem & refactoring #1590#1607dbinkele merged 18 commits intomaintenance/mps20241from
Conversation
…d-Problem-&-Refactoring-#1590 # Conflicts: # code/languages/org.iets3.opensource/languages/org.iets3.variability.artifacts.base/models/org.iets3.variability.artifacts.base.behavior.mps
kbirken
left a comment
There was a problem hiding this comment.
PivotInfo is gone now, congrats! And finally IConfigListVarPoint had to vanish, R.I.P. :-)
Here are some findings from my code review:
- class
Segment- On the first constructor, there is the statement
this.targetNode = targetIVAA.parent;. This might be wrong, as we allow concepts from artifact DSLs to implement interfaceIVarAA, instead of using aNodeAttribute. Use insteadIVAA.artifactRoot(). - This class should be non-mutable, thus it should not not have a
setTargetmethod. - In order to enforce this in future, all private members should be
final(new).
- On the first constructor, there is the statement
- class
AbstractSkelTreeBuilder- Please implement the new code in method
finish()as member methodpublic ArtifactPath asInstanceOnlyPath()inArtifactPath
- Please implement the new code in method
- class
SkeletonNode- I think method
getPathis deprecated long enough and should be deleted now. - The comment in method
getUniqueLabelin the innerif-statement should be updated now as the condition has been changed. - The methods
targetIVAA()andtargetshould be simplified similar tothis.path.segmentsAsList().findLast({it => it.getTarget().isNotNull; }).?getTarget(); - I would prefer if method
setInstance()could be avoided, as it makes the class even more mutable. But the caller seems to be in IETS3.Core...
- I think method
- class
SkeletonTreeGraphCreator- The code
segments.last.getTargetIVAA().artifactName()could be replaced by using the above methodSkeletonNode.getTargetIVAA().
- The code
|
@kbirken All review findings have been addressed. Please have look again |
|
Thanks for addressing the findings, looks good now. I meanwhile found a usage in new code of |
kbirken
left a comment
There was a problem hiding this comment.
The changes look good now. Pls. squash the branch and merge the PR.
solves #1590