Addressing SonarCloud suggestions

rollbar / Rollbar.NET

Rollbar for .NET

https://docs.rollbar.com/docs/dotnet

MIT License

65 stars 44 forks source link

Addressing SonarCloud suggestions #603

Closed akornich closed 2 years ago

akornich commented 2 years ago

Description of the change

Addressing SonarCloud suggestions

Type of change

[ ] Bug fix (non-breaking change that fixes an issue)
[ ] New feature (non-breaking change that adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

n/a

Checklists

Development

[x] Lint rules pass locally
[ ] The code changed/added as part of this pull request has been covered with tests
[x] All tests related to the changed code pass in development

Code review

[ ] This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
[ ] "Ready for review" label attached to the PR and reviewers mentioned in a comment
[ ] Changes have been reviewed by at least one other engineer
[ ] Issue from task tracker has a link to this pull request

akornich commented 2 years ago

There are a lot of comments throughout the code, where 'TODO' has been stripped from the comment, but there's no apparent related code change. I don't know how to interpret these. Did the TODO get implemented in these cases?

Related to that, the Dispose pattern that is implemented several times in the code seems to have a mismatch in meaning between the comments and the code. It was also odd to see a lint error suppression ("S1066:Collapsible "if" statements should be merged") in only one of those cases, and none of the others. Is that lint suppression really needed?

@waltjones , I addressed all of it in the other code-spot-specific comments that you made. Let me know if I missed anything...

waltjones commented 2 years ago

@akornich Thanks for the follow up. The changes in the TODO comments look like something that will come back to haunt later. The resulting comments seem misleading.

akornich commented 2 years ago

@akornich Thanks for the follow up. The changes in the TODO comments look like something that will come back to haunt later. The resulting comments seem misleading.

@waltjones, what do you mean by "haunt later"? These are modified code comments that still have the same directions to follow when/if applicable but do not disturb the SonarrCloud analyzer long-term if never needed to implement in each specific case.

Also, what is exactly so misleading about?:

// free unmanaged resources (unmanaged objects) and override a finalizer below. OR // set large fields to null.

If a class does not use unmanaged resources, there is nothing to free following the first direction. OR If a class does not have any large data fields - no need to set them to null. However, if later on, the class adds the use of a large field, the comment serves as a reminder/direction to set the new field to null.

waltjones commented 2 years ago

If the comments don't apply to the code, they should be removed completely. If they are a todo that should be implemented later, they should say that clearly in some way.

Maybe send the diff to some other people and see what feedback you get.

akornich commented 2 years ago

@waltjones , thanks again for the diligent review!