Refactor TBL to reduce complexity, improve maintainability, reduce technical debt

[skliper]

Is your feature request related to a problem? Please describe.
TBL functions are complex and utilize many modes. Complex enough that minor maintenance is avoided due to the high likelihood of breaking something.

Other code review notes to address as part of the refactor:

• If block could go inside else at line 214
  

  cFE/modules/tbl/fsw/src/cfe_tbl_api.c

  Lines 225 to 230 in 82c1bd4

  	/* Check to make sure we found a free entry in registry */
  	if (RegIndx == CFE_TBL_NOT_FOUND)
  	{
  	Status = CFE_TBL_ERR_REGISTRY_FULL;
  	CFE_ES_WriteToSysLog("CFE_TBL:Register-Registry full\n");
  	}

• Free handle is consumed even if status is already an error (and not returned to the pool)
  

  cFE/modules/tbl/fsw/src/cfe_tbl_api.c

  Line 236 in 82c1bd4

  *TblHandlePtr = CFE_TBL_FindFreeHandle();

• Similar logic in CFE_TBL_Register for single and double buffered tables, factor out duplication

• May be able to shorten lock in CFE_TBL_Register to just the resource allocation parts (although since it's typically just part of startup shouldn't be an issue)

• Multiple returns in CFE_TBL_Load should be refactored out

• CFE_TBL_Manage loop could be a do/while and only loop if additional management required, although really the only two things that could be done in one manage are validate and dump (if an update is scheduled it's first and exits loop), although these are globals so it seems like a validate could run before an update... related to Clarify acceptable actions on a shared table along with expected patterns and add functional tests #1493

• Consider using CFE_TBL_Load from CFE_TBL_LoadFromFile

• CFE_TBL_LoadCmd should use CFE_TBL_LoadFromFile

Describe the solution you'd like
Break down large complex functions.

Describe alternatives you've considered
Note grouped all the suggested refactor changes here, could break out smaller tasks and implement one by one but that may result in extra work.

Additional context
Code review, related to #1493 in that sharing isn't clearly defined

Requester Info
Jacob Hageman - NASA/GSFC

[thnkslprpt]

I have a couple questions, if someone has a minute to answer them.

Point 1: Why is status set to CFE_TBL_ERR_REGISTRY_FULL? It seems all that was checked for is whether the table was found or not, not if the registry was full.

Point 5: Is the preferred method to factor out multiple returns
a) variable status and check if status == CFE_SUCCESS before executing each block of code (i.e. if status has previously been set to an error state, it will skip all following blocks of code to the end)
b) put everything in a while (true) loop and break out where there are currently early returns
c) something else?

[dzbaker]

I have a couple questions, if someone has a minute to answer them.

Point 1: Why is status set to CFE_TBL_ERR_REGISTRY_FULL? It seems all that was checked for is whether the table was found or not, not if the registry was full.

Point 5: Is the preferred method to factor out multiple returns a) variable status and check if status == CFE_SUCCESS before executing each block of code (i.e. if status has previously been set to an error state, it will skip all following blocks of code to the end) b) put everything in a while (true) loop and break out where there are currently early returns c) something else?

@jphickey Would you know the answers?

[jphickey]

This is a big undertaking here -- The problem is that TBL needs a significant overhaul to reduce the complexity of functions. As a guideline the cyclomatic complexity of functions (based on the number of decision points and alternate paths that can be followed) should be kept less than 10. Keeping the functions simple make them automatically more testable, easier to code review, less likely to break things due to unexpected consequences, etc. The biggest offender here by far is CFE_TBL_Register which has a fairly absurd amount of branches, followed by CFE_TBL_Load, but there are many others as well.

The point is not to just make the returns more readable by replacing with a different style, but to split up what is currently a big monolithic function into smaller parts.

The preference is that there are no multiple returns at all after a refactoring - with small/simple functions this becomes easier to achieve. (Except for a possible return early at the beginning as a result of argument/input checks, but nothing in the middle during the business logic of a function - functions should either fail immediately, before any action happens, or run to the end).

That being said, the CFE_TBL_ERR_REGISTRY_FULL status code seems correct, that code is first checking if the table is already registered and if not, it calls CFE_TBL_FindFreeRegistryEntry(), and if there still no defined index after that, it must mean the registry was full.

But this is a great example of hard-to-follow logic buried in the middle of a long function - it is not obvious what's going on here, and this function is over 300 (!!) lines long. The code is very suspicious-looking, because it will enter the next "if" statement (because Status != CFE_TBL_WARN_DUPLICATE evaluates as true), causing it to find a free handle, but throw it away because it eventually skips the rest of the logic.

In short, it's really confusing, and really hard to follow, and nobody wants to touch this code because the chances of unintended consequences are extremely high - for example if that CFE_TBL_ERR_REGISTRY_FULL status code were simply classified as a warning instead of an error, this code would segfault and crash - a completely non-obvious side effect of an innocent-seeming change.

It is this type of possible non-obvious side effects that needs to be resolved by refactoring the code to make it simpler and cleaner.