-
Notifications
You must be signed in to change notification settings - Fork 6k
Modernize code examples for CA rules #48745
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I had a few optional suggestions and then, let's
public ReplyData(Actions action, string reply, bool returnReply) | ||
{ | ||
this.reply = reply; | ||
this.action = action; | ||
this.returnReply = returnReply; | ||
Reply = reply; | ||
Action = action; | ||
_returnReply = returnReply; | ||
} | ||
|
||
// Properties. | ||
public string Reply { get { return reply; } } | ||
public Actions Action { get { return action; } } | ||
public string Reply { get; } | ||
public Actions Action { get; } | ||
|
||
public override string ToString() | ||
{ | ||
return String.Format("Reply: {0} Action: {1} return? {2}", | ||
reply, action.ToString(), returnReply.ToString()); | ||
return string.Format("Reply: {0} Action: {1} return? {2}", | ||
Reply, Action.ToString(), _returnReply.ToString()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could just be a record, and then the body could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BillWagner The example is trying to show why not to pass a reference type as a ref
parameter. I'm not sure if it's as meaningful if the example passes a value type by reference. Do you still feel I should change it? Also, which "body" could be removed?
Example intro:
"The following library shows two implementations of a class that generates responses to the feedback of the user. The first implementation (BadRefAndOut
) forces the library user to manage three return values. The second implementation (RedesignedRefAndOut
) simplifies the user experience by returning an instance of a container class (ReplyData
) that manages the data as a single unit."
docs/fundamentals/code-analysis/quality-rules/snippets/csharp/all-rules/ca1806.cs
Outdated
Show resolved
Hide resolved
docs/fundamentals/code-analysis/quality-rules/snippets/csharp/all-rules/ca1806.cs
Show resolved
Hide resolved
docs/fundamentals/code-analysis/quality-rules/snippets/csharp/all-rules/ca2227.cs
Outdated
Show resolved
Hide resolved
{ | ||
//<snippet1> | ||
public class Mouse | ||
public class Mouse(int numberOfButtons, string scanType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be a record as well:
public record class Mouse(int NumberOfButtons, string ScanType);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BillWagner Does being a record instead of a class make it automatically serializable via binary or XML serialization? If so, then the CA rule wouldn't fire.
Related to #48007.
Requires #48743 to be merged first (for project files).Internal previews