Hi Alex,
I've noticed that with addition of DoubleArrowedLine you've also added concept of "inverted" to attach points. This seems really strange from the perspective that all methods startingPointOf: and endingPointOf: are identical only with swapped ifTrue:/ifFalse: for example ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ startingPointOf: anEdge ^ (inverted ifTrue:[anEdge to] ifFalse:[anEdge from]) position endingPointOf: anEdge ^ (inverted ifTrue: [anEdge from] ifFalse: [anEdge to]) position ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ this is harder to read and more error-prone than ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ startingPointOf: anEdge ^ anEdge from position endingPointOf: anEdge ^ anEdge to position ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ plus there is code duplication. And this is just the simplest attach point. Since I should also add "inverted" to my attach points that are in Roassal (RTRectangleAttachPoint, ...) maybe the actual computation should be delegated? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ RTAttachPoint>>startingPointOf: anEdge ^ inverted ifTrue: [ self basicEndingPointOf: anEdge ] ifFalse: [ self basicStartingPointOf: anEdge ] RTAttachPoint>>endingPointOf: anEdge ^ inverted ifTrue: [ self basicStartingPointOf: anEdge ] ifFalse: [ self basicEndingPointOf: anEdge ] RTAttachPoint>>basicStartingPointOf: anEdge ^ self subclassResponsibility RTAttachPoint>>basicEndingPointOf: anEdge ^ self subclassResponsibility ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This also will not break current implementation but it will give opportunity to improve them. And if there was a situation where the implementation would be actually different for inverted (and not just swapped values), than you could still just implement startingPointOf:/endingPointOf: Thanks, Peter _______________________________________________ Moose-dev mailing list [hidden email] https://www.iam.unibe.ch/mailman/listinfo/moose-dev |
Hi Peter!
This is an excellent idea! I have just implemented it in version 1010 of Roassal. Thanks! Alexandre > On Jul 29, 2015, at 2:04 PM, Peter Uhnák <[hidden email]> wrote: > > Hi Alex, > > I've noticed that with addition of DoubleArrowedLine you've also added concept of "inverted" to attach points. > This seems really strange from the perspective that all methods > startingPointOf: > and > endingPointOf: > are identical only with swapped ifTrue:/ifFalse: > > for example > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > startingPointOf: anEdge > ^ (inverted ifTrue:[anEdge to] ifFalse:[anEdge from]) position > endingPointOf: anEdge > ^ (inverted ifTrue: [anEdge from] ifFalse: [anEdge to]) position > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > this is harder to read and more error-prone than > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > startingPointOf: anEdge > ^ anEdge from position > endingPointOf: anEdge > ^ anEdge to position > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > plus there is code duplication. And this is just the simplest attach point. > > > Since I should also add "inverted" to my attach points that are in Roassal (RTRectangleAttachPoint, ...) > maybe the actual computation should be delegated? > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > RTAttachPoint>>startingPointOf: anEdge > ^ inverted > ifTrue: [ self basicEndingPointOf: anEdge ] > ifFalse: [ self basicStartingPointOf: anEdge ] > > RTAttachPoint>>endingPointOf: anEdge > ^ inverted > ifTrue: [ self basicStartingPointOf: anEdge ] > ifFalse: [ self basicEndingPointOf: anEdge ] > > RTAttachPoint>>basicStartingPointOf: anEdge > ^ self subclassResponsibility > > RTAttachPoint>>basicEndingPointOf: anEdge > ^ self subclassResponsibility > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This also will not break current implementation but it will give opportunity to improve them. > > And if there was a situation where the implementation would be actually different for inverted (and not just swapped values), than you could still just implement startingPointOf:/endingPointOf: > > Thanks, > Peter > > > _______________________________________________ > Moose-dev mailing list > [hidden email] > https://www.iam.unibe.ch/mailman/listinfo/moose-dev -- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;. _______________________________________________ Moose-dev mailing list [hidden email] https://www.iam.unibe.ch/mailman/listinfo/moose-dev |
Free forum by Nabble | Edit this page |