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

some refactorings to reduce cyclomatic complexity #25

Closed
wants to merge 2 commits into from
Closed

some refactorings to reduce cyclomatic complexity #25

wants to merge 2 commits into from

Conversation

jms
Copy link

@jms jms commented Oct 17, 2017

moved some code to functions to reduce the cyclomatic complexity #19

@codecov-io
Copy link

codecov-io commented Oct 17, 2017

Codecov Report

Merging #25 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   94.37%   94.55%   +0.17%     
==========================================
  Files           5        5              
  Lines         249      257       +8     
  Branches       21       20       -1     
==========================================
+ Hits          235      243       +8     
  Misses         12       12              
  Partials        2        2
Impacted Files Coverage Δ
noipy/main.py 92.15% <100%> (+0.85%) ⬆️
noipy/utils.py 95% <0%> (-0.24%) ⬇️
noipy/dnsupdater.py 98.43% <0%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7e30bc...bc76e65. Read the comment docs.

@pv8 pv8 changed the base branch from master to develop November 28, 2017 19:22
@pv8 pv8 changed the base branch from develop to master November 28, 2017 19:23
return auth


def execute_ddns_update(update_ddns, ip, hostname, auth,
Copy link
Owner

Choose a reason for hiding this comment

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

This functions has too many parameters and still has some Cognitive Complexity.

49-51: Function `execute_ddns_update` has 9 arguments (exceeds 4 allowed). Consider refactoring. [structure]
49-73: Function `execute_ddns_update` has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring. [structure]

Maybe try a different approach here.

}


def process_generic(url, updater_options,
Copy link
Owner

Choose a reason for hiding this comment

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

Same issues of execute_ddns_update function (line 49):

77-79: Function `process_generic` has 5 arguments (exceeds 4 allowed). Consider refactoring. [structure]

@pv8
Copy link
Owner

pv8 commented Dec 19, 2017

@jms, thanks for contributing!
I made some comments on your PR. But, although this PR did reduce code complexity in execute_update function, it ends up adding code complexity with the new functions as well.

Running codeclimate analyze noipy/:

== noipy/main.py (2 issues) ==
37-132: Function `execute_update` has a Cognitive Complexity of 40 (exceeds 5 allowed). Consider refactoring. [structure]
37-132: Function `execute_update` has 83 lines of code (exceeds 25 allowed). Consider refactoring. [structure]
== noipy/main.py (6 issues) ==
49-51: Function `execute_ddns_update` has 9 arguments (exceeds 4 allowed). Consider refactoring. [structure]
49-73: Function `execute_ddns_update` has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring. [structure]
77-79: Function `process_generic` has 5 arguments (exceeds 4 allowed). Consider refactoring. [structure]
96-156: Function `execute_update` has a Cognitive Complexity of 15 (exceeds 5 allowed). Consider refactoring. [structure]
96-156: Function `execute_update` has 49 lines of code (exceeds 25 allowed). Consider refactoring. [structure]
96-156: Cyclomatic complexity is too high in function execute_update. (9) [radon]

@jms
Copy link
Author

jms commented Dec 19, 2017

yes, is no good yet, i'll try a different approach and I'll update this pull request.

@pv8 pv8 closed this Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants