extremely minor perf updates #2723
Conversation
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
| [Fact] | ||
| public void InstanceAnnotationNameWithoutNamespaceShouldThrowArgumentException() | ||
| { | ||
| foreach (string name in new[] { ".", "f.", ".f" }) |
There was a problem hiding this comment.
Maybe we should make this test a [Theory] instead of a [Fact] and use the name as parameter instead of iterating through the different input options. With a Theory, the results would clearly indicate which option failed in case some cases fail and some pass.
| @@ -31,6 +31,16 @@ public void InstanceAnnotationNameWithoutPeriodInTheMiddleShouldThrowArgumentExc | |||
| } | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
In the PR description you say:
"I wrote a test to validate that instance annotations of different kind were still written/not written accordingly."
Would you mind adding those test cases to the PR, or are they already covered in our existing test suite?
| if (name.IndexOf('.') < 0 || name[0] == '.' || name[name.Length - 1] == '.') | ||
| // being internal, all callers validate that the name is not empty, but don't validate anything else; here, we validate that there is a namespace, | ||
| // indicated by the presence of a period and by characters before and after the period | ||
| if (name.Length < 3 || name[0] == '.' || name[name.Length - 1] == '.' || name.IndexOf('.', 1, name.Length - 2) < 0) |
There was a problem hiding this comment.
@corranrogue9 In the absence of any data to support this optimization, it could easily cause a regression. In my experience, the most common violation is the absence of a .. It's unlikely to be a leading or trailing .. Which means we might end up introducing 3 comparisons from uncommon/rare scenarios before the comparison for the more common/likely scenario.
While writing instance annotations, we only write annotations if they don't start with
odata.or if they do start withodata.but they aren't in a list of known annotation names. These checks are conceptually correct, but the logic as written requires checking the annotation name forodata.more than once. While updating the code to remove this logic, I wrote a test to validate that instance annotations of different kind were still written/not written accordingly. While doing this, I discovered that theODataInstanceAnnotationtype validates that the name isn't an OData instance annotation in the first place. Because of this, there is no reason to validate the annotation name at all while writing the annotations, so I've removed that unnecessary logic.The
ODataInstanceAnnotationtype also validates that the name is well-formed by checking if it contains a.character to ensure that a namespace is present. It further confirms that the.does not come at the beginning or the end of the name. The code as written would check for the presence of a.and then would check if the first or last character was a.. However, if the first or last character is a., there's no need to do the initial check. I have changed the order of these predicates so that the string is not enumerated unless all of the other conditions are met first.