Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up of this.
Two tests are failing; I will take care of them once initial feedback comes. The problem with these tests is that they expect to be able to set arbitrary IDs to nodes (not just incremental). My solution here does not support this, although it can be modified to support this easily. Do we care about this, though?
Rewriting the code shows some interesting things. In some tests, where an attack graph was being created to create attackers through it via (add_attacker), I removed the attack graph completely, and the tests still succeeded! The reason is that the only thing that
add_attacker
was needed for in those tests was to set a proper attacker ID that was not handled by the Attacker class itself.Also, the role of the
add_attacker
method now, which I still use in this PR, albeit trimmed down, is to find nodes by id. This can be done directly when creating the attacker object. This happens in two places only, and the code's repetition to get nodes by ID would be minimal, especially when #105 is merged. My suggestion thus is to dropadd_attacker
similarly to how this PR droppedadd_node
.The above shows that the attack graph is a thin layer that keeps track of nodes and attackers in its corresponding lists. It is only useful during generation so that nodes get their children and parent lists/sets populated. This is great, but we should honor this in the code, keeping the attack graph as thin as possible and encapsulating node or attacker-related functionality as much as possible in the corresponding classes as his PR does. Then, the
AttackGraph
class could be a class that:An odd exception currently to the above is the
attach_attackers
method. This method reads attackers from the model, creates their objects, adds them to the attacker list, and populates their compromised list. It will be possible to shrink at least when all of the above are addressed or redesigned.A note also, these redesigns typically shrink the size of the code by about 30% relative to the original lines of code that they touch. I think this is great and a good sign usually that the code gets more streamlined removing conceptual loops that were there.