eMoflon / benchmarx

Infrastructure for implementing benchmarx: benchmarks for bidirectional transformation (bx) tools. Also contains a collection of example benchmarx and test runners for various and diverse bx tools.
GNU General Public License v3.0
9 stars 12 forks source link

Review: SDMLib #61

Closed tsdh closed 6 years ago

tsdh commented 7 years ago

Here is my review before I'll submit the evaluation sheet this evening.

I was able to run the solution with the authors' fork, branch SDMLibContribution. The solution passes all tests except for these three:

Since the forked project formats the models in text format all in one line (in contrast to the main benchmarx project), it is hard to spot what the actual problem causing the failures is.

Anyway, what I like with this solution is that it is really fast. Only the BXtend solution is faster (about a factor of 7 for the batch cases and a factor of 2-3 for the incremental ones), and most importantly, it doesn't seem to have scalability issues when the model sizes grow.

Another good point is its conciseness. It consists of only about 100 lines of code when counting just the transformForward() and transformBackward() methods and the few helper methods called from there.

An obviously weak point of the solution is that it is not bidirectional at all, i.e., the forward and backward transformation are completely separate. The only thing they have in common is that they work on the very same kind of models. That said, this can be a reasonable choice in certain scenarios. In this very case, in my opinion, the two preference parameters can be a hindrance for a bidirectional solution because they only have a meaning in the backward direction but their values must not affect the forward direction. Thus, I also had to build one relation which explicitly dispatches on the transformation's direction -- a thing you wouldn't want to have in a truly bidirectional solution.

The visualizations of the pattern are nice and easy to grasp. However, the Java code that you actually have to write could be a bit nicer. Do I see it correctly that some methods like createCondition() get a functional part (the Condition object) and some text for the visualization which doesn't need to have anything to with that Condition?

The decision to manifest the correlations between family members and persons by extending the metamodel with explicit links is a good one which I sadly haven't considered myself for the FunnyQT solution. However, I think the quite operational and less declarative approach provided by SDMLib doesn't work for more complex models. In this case, where we actually just deal with family members and persons, it is quite feasible to use optional subpatterns and NACs to distinguish the case where we already have a matching counterpart and the other case, where we need to create one. But I assume that patterns would quickly become unwieldy in real-world cases. As a transformation writer, I don't want to have to encode this "check if exists or create anew" semantics myself each and every time but want it built into the transformation language.

The solution paper suffices to explain the solution reasonably well, however it doesn't meet the requirements mentioned in the case description. That is, it doesn't mention which test cases it passes expectedly or unexpectedly, and likewise it doesn't mention if the three failing tests fail due to a tool limitation, a bug, or whatever. (Well, it actually doesn't mention that there are failing tests at all.)

So all in all, the solution solves almost all test cases, it is concise and it is really fast. However, it is not bidirectional at all.