consider refactoring the interpolate data section of the prepbufr builder to see if we can appropriately reduce the levels of nesting.

[randytpierce]

We can consider refactoring the interpolate data section of the prepbufr builder to see if we can appropriately reduce the levels of nesting. The build_document method has to

1. Read the prepbufr file.
2. Advance through the subsets.
3. For each subset:
   1. Parse the data defined in the templates.
   2. For the height variable it has to interpolate the heights via the hypsometric equation.
4. After it has interpolated height data along with all the other variables it must interpolate all the variables with logarithmic difference.

The nesting on step 3 can be confusing. Perhaps there is a way to make it flatter.

[ian-noaa]

For my own reference - typically, folks recommend keeping nesting to 2-3 levels deep. They deal with deeper nesting via guard clauses and extracting functions.

Some resources that could be useful:

• Google's Code Health Blog: Nesting guidance
• Google's Code Health Blog: Simplifying Control flow
• Jeff Attwood on "Flattening arrow code"
• Two methods for refactoring deeply nested code

[randytpierce]

After going through all the data comparison to the legacy data and seeing all the problems with interpolation I no longer think it is possible to reduce the complexity of these steps. The heights must be interpolated with the hypsometric equation for every height. Following the hypsometric interpolation the logarithmic interpolation is necessary to get data at the standard levels. Closing this.

[ian-noaa]

This issue was less about reducing the complexity of the steps - those are all very necessary. However, I was hoping we could make the function less nested/easier to understand & update by extracting components into subfunctions. Could we reopen this issue?

As a more trivial example for this section of code:

VxIngest/src/vxingest/prepbufr_to_cb/prepbufr_builder.py

Lines 346 to 357 in 930d7d7

	# create masked array for the variable with ALL the mandatory levels
	# though the levels below the bottom level and above the top level will be masked
	if report == 120 and "wind" in variable.lower():
	# skip this one - it is handled in the 220 report
	continue
	if (
	report == 220
	and variable.lower() != "pressure"
	and "wind" not in variable.lower()
	):
	# skip this one - it is handled in the 120 report - except for pressure
	continue

We could refactor out into a subfunction and use that.

def should_skip_variable(self, report: int, variable: string) -> bool:
  """Contains the rules to determine if a variable should be skipped"""
  if report == 120 and "wind" in variable.lower():
      # skip this one - it is handled in the 220 report
      return True
  if (
      report == 220
      and variable.lower() != "pressure"
      and "wind" not in variable.lower()
  ):
      # skip this one - it is handled in the 120 report - except for pressure
      return True
  return False

We'd then use it in the interpolate_data function like below. It'd then provide one place to put logic for determining if a variable should be skipped:

if self.should_skip_variable(report, variable):
  continue
...

We'd then do this to break out other parts of the function and reduce its nestedness.

I've had some fun asking Copilot how to reduce the nesting for the interpolate_data function. I don't think the way Copilot splits the functions out always makes the most sense. However, it seems like a good starting spot.

[github-actions]

This issue is stale because it has been open 90 days with no activity.