Master Thesis - SCChart Edge Merge - Code Review #7
Conversation
|
|
| */ | ||
| override void processTransition(Transition transition, KEdge edge) { | ||
|
|
||
| if (MERGE_SIMILAR_EDGES.booleanValue) { |
There was a problem hiding this comment.
Documentation on what this hook does and comments about how is accomplished in the code help understand in this file (other than the methods that are overriden and are by that kind of documented already).
There was a problem hiding this comment.
I have added some comments, anything that is still missing sufficient explanation?
Should I remove javadoc comments from the overridden functions?
There was a problem hiding this comment.
In general, anything that is API of new code should be documented so that someone not knowing what some class/method is about gets to know it by reading the JavaDoc for it. Ideally as you did here also with some comments sprinkled inbetween for calculations you do that are not immedeately obvious.
The only thing missing for me would now be a JavaDoc on the class itself explaining what this hook does.
The JavaDoc for overridden functions are imo not necessary, but they do not hurt either, so I would let them in.
There was a problem hiding this comment.
I have now added a doc comment for the class.
I attempted to keep it similar detailed to other hooks' class comment.
I believe this was the only outstanding review comment from you and should therefore be ready for @a-sr to review.
...kieler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/hooks/HideAnnotationHook.xtend
Outdated
Show resolved
Hide resolved
|
If this code should be included in the master, als should then also get a ping (if he did not get that already) to review and include it. |
|
@Eddykasp @NiklasRentzCAU check if this is ready for review by als |
This PR is for the review of the code produced during my master thesis