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

Recommended Code CleanUps #17

Open
TomDonoghue opened this issue Sep 29, 2020 · 0 comments
Open

Recommended Code CleanUps #17

TomDonoghue opened this issue Sep 29, 2020 · 0 comments
Labels
JOSS review https://github.com/openjournals/joss-reviews/issues/2621

Comments

@TomDonoghue
Copy link

As part of the JOSS review (openjournals/joss-reviews#2621) I was scrolling through the code, and I'm collecting some code suggestions. Broadly, I don't consider these strictly necessary for the paper to proceed, but I thought I would collect some notes on things that might be useful for the project.

Code Suggestions:

  • Check for API consistency. I suggest any parameter name referring to the same thing should have the same name across the module
    • For example 'sampling rate' is sfreq in biopeaks/resp but fs biopeaks/fs
    • Just as a note, in my modules, for argument names that are likely to recur a lot, I just keep a list of what they are all called. This can even be collected and made part of contributor materials, so that new code can be written to follow naming standards.
  • Relatedly, consider variable naming. The module mostly uses snake_case, though with some exceptions, such as currentFile and statusBar in view.py. It is also a little inconsistent on if and when words are separated out (with underscores). Labels like prev_peaks and next_peaks are easier to read labels like amppeaks and periodcintp. Some internal names might be able to be safely updated, some might be grandfathered in - but it might be worth trying to set an consistent style from now forwards, for more consistent and easily readable names.
  • Maybe run a linter?
    • There a few randomly really long lines (like in test_heart), which are annoying to scroll / read
    • I consistently run pylint on my modules, checking for any aspects I care about, and it's a nice way to keep code quality good & consistent
  • Check for and remove commented out code
    • There is quite a lot of commented out code, including lots of things that I think relate to debugging, or maybe old. Old versions of things can typically be removed (it is version controlled), or if things are kept for a reason, it might be worth adding a comment and annotating why.
    • If you want to keep debug related code, I might say to have a DEBUG mode. Set a global or something somewhere that you can set, and then these lines can look like if DEBUG: print('doing thing'). This, I think, would be cleaner in the code, and also much easier to use.

I do get that, as a GUI, perhaps many users are less likely to interact directly with the code. But I would still recommend to adopt and continue with good code quality & consistency, both to make it easy for interested users to check the code, to make it easier to onboard people who might want to contribute, and for general maintenance (including for yourself to be able to more quickly interact with the code).

@JanCBrammer JanCBrammer added the JOSS review https://github.com/openjournals/joss-reviews/issues/2621 label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JOSS review https://github.com/openjournals/joss-reviews/issues/2621
Projects
None yet
Development

No branches or pull requests

2 participants