search

denoptim-project / DENOPTIM

DENOPTIM is a software package for de novo design and virtual screening of functional molecules of any kind.
GNU Affero General Public License v3.0
35 stars 10 forks source link

Separate responsibility of Fragment and IAtomContainer #47

Closed einarkjellback closed 3 years ago

einarkjellback commented 3 years ago

Fragment is essentially supposed to work as an Adapter [https://en.wikipedia.org/wiki/Adapter_pattern] for an IAtomContainer. The responsibility of the IAtomContainer is in representing a molecule and the Fragment's responsibility is to allow the IAtomContainer to interact properly with the graph model that DENOPTIM uses. This would suggest that IAtomContainer should be provided as a dependency injection to the Fragment, i.e. you finish building the IAtomContainer before providing it to the Fragment's constructor. This further supported by the fact that after the IAtomContainer is initialized there is, as of writing, no place in the code where it is modified any further.

The immutability of IAtomContainer should be reflected in the code by disallowing any public methods in Fragment to modify the IAtomContainer.

This is not the case however, as there are a number of methods in Fragment that modify the IAtomContainer. Most of these methods have a direct analog in the IAtomContainer's interface (e.g. removeBond(…)). There are, in my opinion, several advantages of making IAtomContainer immutable:

  1. There is a clear separation between the responsibilities of Fragment and of IAtomContainer.
  2. Reduced code bloat as identical methods in IAtomContainer and Fragment are removed.
  3. Looser coupling between Fragment and IAtomContainer. If IAtomContainer changes, Fragment has fewer methods that must be updated to reflect the changes.
  4. Disambiguating the initialization of Fragments. If Fragment in practice never modifies IAtomContainer then it shouldn't be provided as an option either.

My suggestion for fixing this is to make the IAtomContainer-field final and remove any methods in Fragment that modify IAtomContainer.

marco-foscato commented 3 years ago

@einarkjellback: making the IAtomContainer final should be quick enough. Go ahead.

marco-foscato commented 3 years ago

Unfortunately, I now see that making the atom container final would prevent using a reference to identify the atom on which an AP is rooted, and prevents moving the AP and the atoms in space consistently when building a molecular structure out of a graph. @einarkjellback : don't do this.