Improve code style of Send-MailMessage cmdlet #7723
Conversation
Change properties to auto properties Fix comment styling Insert lines after statement with curly braces
ThreeFive-O
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
|
Can those suppress message attributes be removed too? e.g. [SuppressMessage("Microsoft.Naming", "CA1709:IdentifiersShouldBeCasedCorrectly", MessageId = "Cc")]
[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]Haven't seen them in newer cmdlets. I guess some artefacts from the original code base. |
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
LGTM with one minor comment.
| } | ||
| } | ||
| private SwitchParameter _bodyashtml; | ||
| public SwitchParameter BodyAsHtml { get; set; } = false; |
There was a problem hiding this comment.
Is the init really needed?
There was a problem hiding this comment.
Not really needed. It would be false anyway. Just make it more explicit. Same for the other inits.
I've seen it in other cmdlets and adopted this style.
There was a problem hiding this comment.
Actually, C# coding guidelines discourage initializing properties and fields to the default value. Thus, you don't initialize int to zero, references to null, and fields/properties to false. A SwitchParameter is a good example of this.
dantraMSFT
left a comment
dantraMSFT
left a comment
There was a problem hiding this comment.
A couple of minor comments/nits; otherwise, looks good.
| } | ||
| } | ||
| private SwitchParameter _bodyashtml; | ||
| public SwitchParameter BodyAsHtml { get; set; } = false; |
There was a problem hiding this comment.
Actually, C# coding guidelines discourage initializing properties and fields to the default value. Thus, you don't initialize int to zero, references to null, and fields/properties to false. A SwitchParameter is a good example of this.
|
|
||
| /// <summary> | ||
| /// Specifies a value indicating whether the mail message body is in Html. | ||
| /// Gets or sets a value indicating whether the mail message body is in Html. |
There was a problem hiding this comment.
Minor nit... To be consistent with the other property summaries, use
'Gets or sets the value...'
versus
'Gets or set a value...'
Fix summary comment for BodyAsHtml
|
@dantraMSFT and @iSazonov Thank you for the review and the help.
|
4 participants
PR Summary
This PR should improve the code readability for the cmdlet, before any other work is started. Changes:
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.or can only be tested interactively[feature]if the change is significant or affects feature tests