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

Reduce indentation by inverting if conditions #357

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

troyraen
Copy link
Contributor

@troyraen troyraen commented Dec 5, 2024

Some of the spectroscopy functions have quite a few levels of indentation (almost 10 in some cases) due to nested for loops and if statements. This tends to squish the code into a narrow region on the right making it more difficult to read. This PR reduces some of the indentation by inverting the conditions of if statements where possible.

Simple example with just 2 levels (benefits are greater when there are more levels). This:

for sample in sample_table:
    table = query_archive(...)
    if len(table) > 0:
        # long block of code
        # ...
    else:
        print("no results found")

becomes this:

for sample in sample_table:
    table = query_archive(...)
    if len(table) == 0:
        print("no results found")
        continue
    # long block of code. now has more room horizontally.
    # ...

@troyraen troyraen added the use case: spectroscopy Spectroscopy use case label Dec 5, 2024
@troyraen troyraen marked this pull request as ready for review December 5, 2024 03:09
@troyraen troyraen requested a review from bsipocz December 5, 2024 03:10
@bsipocz
Copy link
Member

bsipocz commented Dec 5, 2024

Some of these conditionals and loops are also very long, and for those the rule of thumb I was taught was to avoid any such section that is longer than one editor screen. (aka refactoring is advised if/when one ends in that situation)

@troyraen
Copy link
Contributor Author

troyraen commented Dec 5, 2024

@bsipocz I totally agree, just trying not to spend that much time on this code right now. The changes I made here were some low-hanging fruit that will make the rest of my review a little easier. This tends to happen a lot with these notebooks, and I think it would be good for us to come up with a small number of relatively simple things the scientists can think about / do to avoid ending up in this situation with future code.

@bsipocz
Copy link
Member

bsipocz commented Dec 5, 2024

Yes, I totally meant it for that simple list of guidelines, rather than to fully rewrite of what we have. (OTOH, you practically did a couple of the refactorings in this PR, so we can even link to this as real life example of how to refactor a long conditional by e.g. inverting)

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks correct, thanks!

@bsipocz bsipocz merged commit 63d4dfa into main Dec 5, 2024
3 checks passed
@bsipocz bsipocz deleted the raen/simplify/spectra_generator branch December 5, 2024 03:49

# If multiple entries are found, pick the closest.
# Or should we take the average instead??
if len(tab) > 0:
Copy link
Contributor Author

@troyraen troyraen Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code block (lines 50-62) is weird in two ways:

  1. It seems to think the condition was if len(tab) > 1: instead of if len(tab) > 0: (which would make more sense given that the == 0 case was already handled).
  2. The resulting variable tab_final is not used. Maybe the following line of code was supposed to use it instead of tab?

The diff in this PR makes it difficult to look at, and I'd rather not make substantive changes in this PR anyway. Opening an issue to be addressed separately. (#358)

github-actions bot pushed a commit that referenced this pull request Dec 5, 2024
Reduce indentation by inverting `if` conditions 63d4dfa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
use case: spectroscopy Spectroscopy use case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants