Addressing SonarCloud suggestions

[akornich]

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

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• 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

[waltjones]

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]

Same here as above. Is this supposed to be a TODO, or is the thing in the comment already implemented now?

[akornich]

this was already addressed by my comment/reply above...

[akornich]

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]

@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]

@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]

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]

@waltjones , thanks again for the diligent review!