Molecular Inorganics

[JanCBrammer]

No description provided.

[JanCBrammer]

• Consider reverting the style changes (the style changes are nicely isolated in a single commit that can be dropped). The style that's introduced deviates from the conventions in the remainder of the codebase. E.g., in the existing codebase, pointer variables are declared type* name, whereas the changes declare type *name, i.e., the placement of the asterisk is inconsistent.
  Note to myself and other reviewers: since the style changes make up a majority of the diffs a lot of effort goes into scanning the diffs for semantic changes (aka the functionality that's implemented). To view only the semantic changes check the "Hide whitespace" box in the review configuration (https://github.blog/changelog/2021-10-14-hiding-whitespace-is-now-remembered-for-each-pull-request/).

• As discussed, we need to introduce unit-level testing along with this feature.

• The comments (both inline and function level) could focus less on the implementation ("how", e.g., "variable initialization for the Organometallics parameter") and more on what the code does ("why and what", a nice example is the comment regarding the OrgMetElData).

• Remove code instead of commenting it out (including debugging).

• Consider moving the feature to a dedicated file. I feel like the string utilities might not be the most intuitive place to implement the feature.

• Consistently use either snake_case, UpperCamelCase, or lowerCamelCase.

[nnuk]

• @JanCBrammer you mentioned about the style changes. Actually, the use of type *name is more preferred method as multiple declarations are handled e.g. type *name1, *name2. But in case of current style it will be type* name1, name2; which means name2 is just the type and not pointer. As I didn't want to use my time correcting those so I at least wanted to use the preferred style.

• Next I will be trying to remove the commented debugging statements.

• The inline comments mostly focus on what the specific line of code is doing because it makes debugging somewhat easier. But for sure few comments need to be removed like the one you mention about the parameter.

[djb-rwth]

• @JanCBrammer you mentioned about the style changes. Actually, the use of type *name is more preferred method as multiple declarations are handled e.g. type *name1, *name2. But in case of current style it will be type* name1, name2; which means name2 is just the type and not pointer. As I didn't want to use my time correcting those so I at least wanted to use the preferred style.

Microsoft Visual Studio editor is also partially responsible for this as it does some automated formatting on-the-fly.

• Consider moving the feature to a dedicated file. I feel like the string utilities might not be the most intuitive place to implement the feature.

Agreed. IMHO, all molecular inorganic code should be placed in a separate file (w/accompanying header) in order to avoid confusion regarding which file contains what. The OrgMetArray matrix (or whatever it shall be called) and a few related functions seem somewhat out-of-place in strutil.c as this file considers various utility functions apart from metal disconnection/reconnection. Also, it would be more beneficial for continuous monitoring of development and/or applying bug fixes.

• Consistently use either snake_case, UpperCamelCase, or lowerCamelCase.

This mess unfortunately exists throughout the code...

[nnuk]

Agreed. IMHO, all molecular inorganic code should be placed in a separate file (w/accompanying header) in order to avoid confusion regarding which file contains what. The OrgMetArray matrix (or whatever it shall be called) and a few related functions seem somewhat out-of-place in strutil.c as this file considers various utility functions apart from metal disconnection/reconnection. Also, it would be more beneficial for continuous monitoring of development and/or applying bug fixes.

I will try to work on seperating molecular inorganics in parallel with building current functionality. This way I can be assure that everything works perfectly well and there's no wasting sweet time. But, until then I think we can take the current code.

[schatzsc]

@nnuk

I will try to work on seperating molecular inorganics in parallel with building current functionality. This way I can be assure that everything works perfectly well and there's no wasting sweet time. But, until then I think we can take the current code.

When you do this, please remove all references to "OrgMet" also from the code - talk to Sonja, Jan, and Gerd how to more properly name it. But it must really go from all comments, variables, function names, and so on

[nnuk]

@nnuk

I will try to work on seperating molecular inorganics in parallel with building current functionality. This way I can be assure that everything works perfectly well and there's no wasting sweet time. But, until then I think we can take the current code.

When you do this, please remove all references to "OrgMet" also from the code - talk to Sonja, Jan, and Gerd how to more properly name it. But it must really go from all comments, variables, function names, and so on

@schatzsc, after discussions the terminology change to Molecular Inorganics has already been done at my end from comments, variables, functions and parameter. You will be able to see that in the coming time.

Withdrawing review request.

[cm-beilstein]

@nnuk Can you resolve the conflicts and merge the pull request back into dev? Thanks

[nnuk]

@nnuk Can you resolve the conflicts and merge the pull request back into dev? Thanks

@cm-beilstein Sure, I am already working on that.