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

Improve gocyclo from 43 to 24 for blockwise processReceivedMessage #275

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

arun1587
Copy link
Collaborator

@arun1587 arun1587 commented Jan 9, 2022

  • no functional changes.
  • moved the following logic to separate functions.
    -- handling observe response
    -- fetching the received message from the cache
    -- fetching the payload from the received message
    -- sending the response
  • blockwise unit tests passed

could be better if we can bringdown the gocyclo index to 15. currently it is at 24.

msgGuard = cachedReceivedMessageGuard.(*messageGuard)
err := msgGuard.Acquire(msgGuard.Context(), 1)
if err != nil {
return nil, 0, fmt.Errorf("handlerReceivedCache: cannot lock message: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

the label in fmt.Errorf handlerReceivedCache doesn't correspond to the function name.

EDIT: same for additional two places in this function, please fix that and we can merge it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

nits: *sended* -> *sent* to be changed in logs, error messages, function names and var names across this library.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've run misspell over the code, but there's still plenty of spelling mistakes left. Perhaps there's some other tool I can integrate to check the code. Anyway, feel free to fix a mistake when you see it.

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.

None yet

2 participants