Refactor builder and Implement builder for DFD#92
Conversation
|
For this PR, I would prefer if we perform a thorough code review, because there is some code duplication in the changes i do not know how to avoid |
|
@Entenwilli As there are a lot of changes, could you highlight the critical spots for us? For example, just by adding comments to the code in this PR. |
...src/org/dataflowanalysis/analysis/pcm/builder/PCMDataFlowConfidentialityAnalysisBuilder.java
Outdated
Show resolved
Hide resolved
...wanalysis.analysis/src/org/dataflowanalysis/analysis/builder/validation/ValidationError.java
Outdated
Show resolved
Hide resolved
...rg.dataflowanalysis.analysis.tests/src/org/dataflowanalysis/analysis/tests/dfd/BaseTest.java
Show resolved
Hide resolved
|
Added some comments about parts to review @Nicolas-Boltz @sebinside |
Entenwilli
force-pushed
the
dfd-builder
branch
from
07d7c50 to
d314088
Compare
sebinside
left a comment
sebinside
left a comment
There was a problem hiding this comment.
I had a quick look at the changes and played with code in Eclipse. I'm happy with new look and feel of the builders.
Besides the already ongoing discussion, I only have two additional points:
- The refactoring into three separate projects: I like it, but I would also like to hear @Nicolas-Boltz opinion on that as he recently already cleaned up the project
- There seems to be some code duplication in the resource provider, e.g.
findElementandlookupElementWithId. As these are technical and hard-to-understand methods, we should avoid this.
… plugin BREAKING CHANGE: Package namespaces for pcm elements moved
This commit adds validation results for the builder to output errors for every validation error
BREAKING CHANGE: Namespace changed for dfd components
Entenwilli
force-pushed
the
dfd-builder
branch
from
3cf55d6 to
9f29e68
Compare
|
@sebinside @Nicolas-Boltz This PR is now ready to be reviewed again 🎉 |
sebinside
left a comment
sebinside
left a comment
There was a problem hiding this comment.
Thank you for the quick reaction and for addressing our concerns! I think the only thing missing is the replacement of the AnalysisData class by its content. But I would opt for addressing this in a subsequent PR in order to merge this PR as fast as possible :)
3 participants
This PR refactors and simplifies the builder for creating subtypes of the dataflow analysis. Furthermore, it implements the builder for the DFD Analysis.
This PR resolves #87, #102 and #88