|[mdt-ocl.dev] Impact Analyzer review|
That's very encouraging as regards functionality. Just need the review.
Because the code is largely the work of a single author, and no other committer has read more than a few lines, I am keen that it should be reviewed. From my own experience I am aware that code evolves and not all the revisions that are desirable actually happen. The IA code has certainly evolved, so I expect that at least a minor clean up is desirable.
By review, I mean, pay attention to (in decreasing priority)
This is very difficult to change once promoted so it wants to be right. My understanding is that there is a very small 'facade' so the external API is small. It's clearly useable, but could or should it be better? Is it too fat or too thin? Is it extensible?
This too can be difficult to change if internal APIs are not constrained. How much functionality is exposed to arbitrary derived classes by protected access. Is this necessary? Can it be shut off? Can a minor refactoring support extension more easily?
Can inefficient O(N*N) algorithms be replaced by O(N)? Can O(N) be replaced by O(1)? Are results sensibly cached in variables?
For each field/variable, is the lifetime of the variable compatible with its validity. i.e. is the status appropriately updated in response to a Resource change, and is memory leakage avoided? Resource change is badly handled by OCL at present, so it may be that for now there is a top level Javadoc mandating total reconstruction after a change. One day the IA might monitor resources and automatically update itself.
plugin and other global names
These obviously change. The simplest change is just delete ".examples". Do we want to do something else?
It would be nice if the event plugins went to EMF, but that doesn't look likely, so they too need review.
It would be nice to have names that can accommodate the pivot model sometime. I would like to try to partition the code into the run-time code that performs (re-)evaluation and the meta-run-time code that maintains the control objects that make IA so good. If this is possible, then we want corresponding names. Perhaps
I hope that migration of the run-time code to align with the code generated Java can be done quite easily, since the code generated Java makes no use of any form of the OCL meta-model; just the polymorphic Values and polymorphic Domain model for which there is direct and Reflective Ecore support. Perhaps
Migration of the meta-run-time code will be harder because that obviously makes use of the OCL meta-model. Perhaps
In order to avoid code duplication, code that is independent of Ecore/UML/Pivot should be in perhaps
It may also be appropriate to place some declarations independent of Ecore/UML/Pivot such as extension points in
We cannot easily use org.eclipse.ocl since that is highly Ecore/UML dependent.
NB being independent of Ecore does not prohibit use of EObject, EObject.eClass() etc. I hope that the external API facade can be Ecore/UML/Pivot independent and so in org.eclipse.ocl.common.impact.
Are package, class, method names and to a lesser extent field names consistent and readable?
The new direct OCL to Java code generator can be invoked automatically from genmodel. Addition of some GenAnnotation could control how an IA instantiation was auto-generated. It would be good if this relatively simple interface could be thought through so that it is easy to implement after Juno, if not implemented for Juno.
There are many JUnit tests. Is the coverage adequate? Are all of the tests really useful? Can the tests be combined into one overall ImpactAnalyzer test suite? Is a memory leakage test needed?
Is the code generally readable? I don't expect rigid application of precise formatting guidelines; just avoid anything too inconsistent with default Eclipse layout.
Some review comments can probably be resolved very quickly and so just done.
Anything affecting API needs to be done as soon as possible.
Anything affecting accuracy should be done soon, but some things may be hard and so left in a Bugzilla.
It would be good to tackle performance soon as well, but what cannot be done should go to Bugzilla.
Please branch the code on GitHub and commit your refactorings and distinctively tagged comments such as // REVIEW or // BUG. Please avoid mixing name refactorings and review comments in the same commit, since refactorings tend to produce large deltas; individual comments will get lost. (One day we might have Gerrit).
On 03/02/2012 10:23, Michael Golubev wrote:
Back to the top