#473 - Specify which class the graph is for. #480
Conversation
|
This pull request #480 is assigned to @victornoel/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job |
| this.nds = new Unchecked<>( | ||
| new Sticky<>( | ||
| () -> XmlGraph.build(skeleton) | ||
| () -> XmlGraph.build(skeleton, pname, cname) |
There was a problem hiding this comment.
@vzurauskas why is it better to make XmlGraph for one class? Does it mean we need to introduce a todo to create another class responsible for iterating on all the classes of this skeleton?
There was a problem hiding this comment.
@victornoel XmlGraph implements this interface:
public interface Graph {
List<Node> nodes();
}It's a list of nodes for one class, because if it was for all classes in skeleton, it would have to be a list of lists of nodes, or a map to lists of nodes. As far as I can tell, the graph is useful only in a context of a single class, because I don't think there are any cross-class cohesion metrics.
Iteration could be metric-specific, I guess. E.g. Ccm iterates through classes of a transformed skeleton and has a method (not implemented currently) to update the NCC value using a graph of a specific class.
There was a problem hiding this comment.
@vzurauskas ok, thank you for the explanation :)
| skeleton.xml().nodes("//class").get(0), | ||
| skeleton.xml() | ||
| .nodes( | ||
| new Joined("", "//package[@id=", pname).toString() |
There was a problem hiding this comment.
@vzurauskas is it normal that the [ is not closed at the end with a ]?
There was a problem hiding this comment.
@victornoel No, I don't think that's normal. Fixed.
There was a problem hiding this comment.
@vzurauskas just to understand, this bug wasn't caught because there are no tests? If there is no todo already, let's add one to add more tests then, WDYT?
There was a problem hiding this comment.
@victornoel There are tests in XmlGraphTest, but they are disabled now. There's a ticket to enable them, it's the other PR you're reviewing. :)
There was a problem hiding this comment.
@vzurauskas ah perfect then :)
| skeleton.xml().nodes("//class").get(0), | ||
| skeleton.xml() | ||
| .nodes( | ||
| new Joined("", "//package[@id=", pname).toString() |
There was a problem hiding this comment.
@vzurauskas I think using a FormattedText would not make this much more readable instead of Joined here and in other places.
There was a problem hiding this comment.
@victornoel Good idea.
|
@paulodamaso this is ok to be merged |
|
@victornoel/z this job was assigned to you 14days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
|
@vzurauskas now there are conflicts :) |
|
@victornoel Fixed. @paulodamaso This can be merged now. |
|
@paulodamaso ping |
|
@victornoel @vzurauskas Sorry for the absence guys |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@paulodamaso Done! FYI, the full log is here (took me 14min) |
|
The project doesn't have enough funds, can't make a payment |
|
Code review was too long (41 days), architects (@paulodamaso) were penalized, see §55 |
#473
Made XmlGraph specific to a particular class inside a skeleton.