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

Consider improving code style and adopting style guidelines #34

Open
lcniel opened this issue Jun 20, 2024 · 0 comments
Open

Consider improving code style and adopting style guidelines #34

lcniel opened this issue Jun 20, 2024 · 0 comments

Comments

@lcniel
Copy link
Contributor

lcniel commented Jun 20, 2024

There are many issues and inconsistencies with the current style of the code, many of them due to enforcing compatibliity with Python 2 previously. The code is deceptively complicated due to the asynchronous nature of many classes (making heavy use of queue, thread, etc), which makes clear coding style crucial in keeping the code maintainable.

I will focus here on what I regard as the largest problems:

  • The names of many variables use Python default function names, such as id and type. Apart from potentially affecting code execution this also disrupts highlighting in editors.
  • There are few to no type hints in the code, which among other things leads to some confusing methods, such as the recurring sampler.do_sample() which is a boolean, and it is often difficult to sort out whether e.g. an identification number should be given as a string or as an integer.
  • There are many conditionals of the form
if some_variable:
   ...

where some_variable is not necessarily or obviously a bool. These conditionals can give rise to unexpected behaviour and are uninformative to the reader, because they do not clarify what the expectation is. In more than a few cases, the variable is e.g. a string which will return False when the string is empty, but the code actually initializes the string as None, which also yields False. This kind of case should be clarified with a clause like some_variable is None. len(some_variable) == 0, some_variable == '', and so on.

A particularly egregious example of confusing conditionals is in sams.sampler.SlurmInfo.py:

        if not self.do_sample():
            self.store(self.data)

One would expect self.do_sample == True to imply that we should take a sample, but that is not the case in this code... I think the logic here is that if do_sample is True then a sample was already taken, and we do not need a second one, so we only take a sample if it is False or None (or any of the other values that will yield False...). The code follows an internal logic that is quite hard to figure out in this case.

  • The code contains a ready mixture of map and filter, sometimes mixed with list comprehension. These can all be replaced with list comprehension only, which is munch more semantically clear.

These are what I would regard as the main íssues which are not more generic (e.g., I also think many methods follow questionable naming schemes) or more preference-oriented (e.g., I think one should use dict() and f-strings whenever possible, and that one should use compact rather than exploded brackets) although I believe the latter should also be clarified at some point as the code is not consistent on these points.

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

1 participant