Prefer null conditional over ternaries #75
Conversation
|
|
||
| [TestCase("var firstName = person.Age != null ? person.FirstName : null;")] | ||
| [TestCase("var firstName = person.Age == null ? null : person.FirstName;")] | ||
| [TestCase("var firstName = person.Age.HasValue ? person.FirstName : null;")] |
There was a problem hiding this comment.
Should there be a test for the case where the ternary checks !person.Age.HasValue?
There was a problem hiding this comment.
Are these examples of code that would trigger the diagnostic and should be changed? What's the desired replacement? (I don't see how the null-coalescing operator would be used here; what am I missing?)
There was a problem hiding this comment.
Added better tests here. These were certainly sloppy, tried to get them in quick before I ran out of time at the end of the day, no real excuse.
var firstName = person.Age?.BirthYear; <-- This should better catch all the HasValue cases.
I have not removed the == null and != null because I still see some value in having them (ease of deployment), but I'm happy to rip those checks out for sure, as they can lead to issues down the road in terms of maintenance or even bugs.
There was a problem hiding this comment.
FWIW, you can set a PR to Draft status if you know it's not fully-baked.
|
|
||
| protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() => new NullTestTernariesAnalyzer(); | ||
|
|
||
| private const string c_preamble = @""; |
There was a problem hiding this comment.
Given that tis preamble is empty, you should probably delete it and all its uses. At the very least, we no longer allow unnecessary verbatim strings ;)
There was a problem hiding this comment.
Cleaned this up.
Co-authored-by: bagley2014 <[email protected]>
…om/jeffcorcoran/FaithlifeAnalyzers into PreferNullConditionalOverTernaries
| [TestCase("var firstName = person != null ? person.FirstName : null;")] | ||
| [TestCase("var firstName = person == null ? null : person.FirstName;")] |
There was a problem hiding this comment.
These are already handled by IDE0031; we shouldn't create (or maintain) functionality that duplicates built-in analysers.
There was a problem hiding this comment.
(These tests shouldn't just be deleted; they should be moved to the "good" test (even though they're not good code) to verify that the analyser isn't alerting on them.)
| private static readonly DiagnosticDescriptor s_rule = new( | ||
| id: DiagnosticId, | ||
| title: "Null Checking Ternaries Usage", | ||
| messageFormat: "Prefer null conditional operators over ternaries explicitly checking for null", |
There was a problem hiding this comment.
| messageFormat: "Prefer null conditional operators over ternaries explicitly checking for null", | |
| messageFormat: "Use null propagation", |
The message from IDE0031 is simpler and appropriate.
|
This analyser would really benefit from a Fix Provider, since the transformation is conceptually straightforward (AFAIK). |
This PR hopes to resolve #57 by checking for null values / checks in ternary statements, as well as looking for HasValue, and requesting the user instead do a null conditional operator for cleaner code.