sbmlteam / jsbml

JSBML is a community-driven project to create a free, open-source, pure Java™ library for reading, writing, and manipulating SBML files (the Systems Biology Markup Language) and data streams. It is an alternative to the mixed Java/native code-based interface provided in libSBML.
https://sbml.org/software/jsbml/
GNU Lesser General Public License v2.1
37 stars 24 forks source link

Extend TreeNodeWithChangeSupport with Visitor pattern #262

Open Schmoho opened 2 months ago

Schmoho commented 2 months ago

This is a suggestion: It seems to me that the current implementation does not support the idiomatic way to perform tree walks in Java, because the TreeNode interface that is used does not support the Visitor pattern.

That is, doing a tree walk now requires to explicitly handle type dispatch in the walking class, which is kind of painful to do when also accounting for Plugins and Version differences.

I am very uncertain whether my current understanding of JSBML is correct in that regard and also I am kind of shaky on Java patterns, so please feel free to correct me.

Since the TreeNodeWithChangeSupport interface is the central interface which allows to perform generic tree walks on SBMLDocuments, I would suggest to add at least one interfacesMutatingTreeNodeVisitor with signatures public void visit (Species s) etc. TreeNodeWithChangeSupport would then have void accept(MutatingTreeNodeVisitor v).

draeger commented 2 months ago

Thank you for this interesting suggestion. In my point of view, the TreeNode interface that is on the top of JSBML's type hierarchy was placed in the wrong Java package. There should have been a general algorithm or patterns package but instead this important interface is part of the SWING graphical user interface package. Your idea of making the derived interface TreeNodeWithChangeSupport more compatible with recent developments seems sound. However, modifications to a top-level generic interface may have many (potentially also unwanted) effects. In addition, an implementation needs to provided in one of the implementing abstract classes (or a default implementation if suitable). Could you elaborate what subsequent changes to the type hierarchy or implementing classes would be needed?