Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate responsibility of Fragment and IAtomContainer #47

Closed
einarkjellback opened this issue Apr 9, 2021 · 2 comments
Closed

Separate responsibility of Fragment and IAtomContainer #47

einarkjellback opened this issue Apr 9, 2021 · 2 comments
Assignees

Comments

@einarkjellback
Copy link
Contributor

einarkjellback commented Apr 9, 2021

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
Copy link
Member

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

@marco-foscato marco-foscato removed the good first issue Good for newcomers label Apr 11, 2021
@marco-foscato
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants