ideaconsult / jna-inchi

Wrapper to access InChI from Java
GNU Lesser General Public License v2.1
0 stars 0 forks source link

FileTextUtils - splitting and general issues #8

Closed uli-f closed 1 year ago

uli-f commented 1 year ago

FileTextUtils is a long, monolithic class with lots of code.

If I understand correctly, it does two things:

Even though related these are two different objectives and I would prefer if this class can be split into two (or potentially three) classes:

uli-f commented 1 year ago

The member private ReactionFileFormat autoRecognizedFormat is initialized with null, never changed and only ever queried for being equal to ReactionFileFormat.RD which evaluates to false.

Am I missing something? Or is there something missing in the code?

ntk73 commented 1 year ago

FileTextUtils is a long, monolithic class with lots of code.

Even though related these are two different objectives and I would prefer if this class can be split into two (or potentially three) classes:

* one for consuming reaction files and producing a `RinchiInput` instance

* one for consuming `RinchiInput` and producing a reaction file

* any shared functionality can go in a shared class (if any)

Yes, this is a good idea. I could do it. After I merge this pull request (or eventually some other ones :-) )

ntk73 commented 1 year ago

I am planning to start working on splitting the FileTextUtils class. Let us agree for the new class names. I would suggest: MDLReactionReader and MDLReactionWriter and both classes to be put in cheminfo sub-package (folder) I do not mention explicitly RXN because RXN and RDFile are both supported as MDL formats. If there is anything else, common for both classes, it could stay in the original class FileTextUtils, which also could be moved in cheminfo.

ntk73 commented 1 year ago

I would ask you while working on this issue and some other issues, to stop making PRs (if any), to avoid conflicts

uli-f commented 1 year ago

I would suggest: MDLReactionReader and MDLReactionWriter and both classes to be put in cheminfo sub-package (folder) I do not mention explicitly RXN because RXN and RDFile are both supported as MDL formats. If there is anything else, common for both classes, it could stay in the original class FileTextUtils, which also could be moved in cheminfo.

I am happy with MDLReactionReader and MDLReactionWriter. Putting them in the cheminfo package is a good idea, too :)

If there is leftover code with functionality shared between MDLReactionReader and MDLReactionWriter I would prefer a name that makes the close association clear, e.g. MDLReactionUtils...? And yes, moving that class in the cheminfo package would be perfect!

I would ask you while working on this issue and some other issues, to stop making PRs (if any), to avoid conflicts

Understood 👍🏼

ntk73 commented 1 year ago

If there is leftover code with functionality shared between MDLReactionReader and MDLReactionWriter I would prefer a name that makes the close association clear, e.g. MDLReactionUtils...? And yes, moving that class in the cheminfo package would be perfect!

MDLReactionUtils sounds good (lets see what leftovers will be there :-) )

ntk73 commented 1 year ago

FileTextUtils class is replaced with the 3 classes we agreed for: MDLReactionReader, MDLReactionWriter and MDLReactionUtils.

uli-f commented 1 year ago

Thank you! That reads a lot easier now that the two intents reading and writing are separated from each other in different classes.