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

Lowering Cognitive complexity as per SonarCloud #58

Open
AlexPatrie opened this issue Apr 18, 2023 · 0 comments · May be fixed by #57
Open

Lowering Cognitive complexity as per SonarCloud #58

AlexPatrie opened this issue Apr 18, 2023 · 0 comments · May be fixed by #57
Labels
discussion documentation Improvements or additions to documentation

Comments

@AlexPatrie
Copy link
Contributor

Justification
After several attempts of pushing type annotations to biosimulators_copasi with a PR, I have been repeatedly met with an error from SonarCloud which indicates that a given function’s “cognitive complexity” is above the acceptable threshold. The documentation specifies that it will assess a negative mark for a variety of statements and situations including: nesting, recursion, and any statement that includes if/else/while/except/and/or/is/not. Exceptions to this rule are made with try/finally blocks and boolean dictionaries.

Examples:
For example, I was able to devise a solution that satisfied the bot while still returning the desired result for the optional keyword argument “config”, where the default is None:

Originally: (+1 cognitive complexity)
config = config or get_config()

Revised: (+0 cognitive complexity)
config = {True: config, False: get_config()}.get(bool(config), config)

Clearly, this solution is neither scalable nor ‘Pythonic’ in nature. Another source of SonarCloud contention pertains to nested-if statements whose depth of nesting is > 1. For example:

` for change in model.changes:
target_sbml_id = model_change_target_sbml_id_map[change.target]
copasi_model_obj = get_copasi_model_object_by_sbml_id(copasi_model, target_sbml_id, units)
if copasi_model_obj is None:
invalid_changes.append(change.target)
else:
model_obj_parent = copasi_model_obj.getObjectParent()

              if isinstance(model_obj_parent, COPASI.CCompartment): 
                  set_func = model_obj_parent.setInitialValue
                  ref = model_obj_parent.getInitialValueReference()
  
              elif isinstance(model_obj_parent, COPASI.CModelValue): 
                  set_func = model_obj_parent.setInitialValue
                  ref = model_obj_parent.getInitialValueReference()
  
              elif isinstance(model_obj_parent, COPASI.CMetab): 
                  if units == Units.discrete: 
                      set_func = model_obj_parent.setInitialValue
                      ref = model_obj_parent.getInitialValueReference()
                  else:
                      set_func = model_obj_parent.setInitialConcentration
                      ref = model_obj_parent.getInitialConcentrationReference()
  
              model_change_obj_map[change.target] = (set_func, ref)`

Possible solutions:
One effective solution to this could include a guard clause. The proposal at hand involves refactoring nested conditional statements whose traversal depth is > 1. The above code is a perfect example of this cognitive complexity. If setup required python >= 3.10, then the implementation of match/case statements in place of ternary-if statements would satisfy the requirements of this issue.

@AlexPatrie AlexPatrie assigned AlexPatrie and unassigned AlexPatrie Apr 18, 2023
@AlexPatrie AlexPatrie linked a pull request Apr 18, 2023 that will close this issue
@AlexPatrie AlexPatrie added discussion documentation Improvements or additions to documentation labels Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion documentation Improvements or additions to documentation
Development

Successfully merging a pull request may close this issue.

1 participant