Add GetConnectionIdFromUri Method to ConnectionIdHelper #5231
Conversation
ugras-ergun-sonarsource
changed the base branch from
master
to
feature/sloop-rule-meta-data
ugras-ergun-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| @@ -33,6 +35,22 @@ internal class ConnectionIdHelper : IConnectionIdHelper | |||
| private const string SonarQubePrefix = "sq|"; | |||
| private static readonly Uri SonarCloudUri = new Uri("https://sonarcloud.io"); | |||
|
|
|||
| public string GetConnectionIdFromUri(string uriString, string organisation) | |||
There was a problem hiding this comment.
BoundSonarQubeProject has Uri property, why do you use string here? The other method of this class also returns Uri and not string
ugras-ergun-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| @@ -33,6 +35,22 @@ internal class ConnectionIdHelper : IConnectionIdHelper | |||
| private const string SonarQubePrefix = "sq|"; | |||
| private static readonly Uri SonarCloudUri = new Uri("https://sonarcloud.io"); | |||
|
|
|||
| public string GetConnectionIdFromUri(Uri uri, string organisation) | |||
| { | |||
| if (uri is not null) | |||
There was a problem hiding this comment.
invert if to reduce nesting
There was a problem hiding this comment.
I don't think it will be that useful because then we'll need to have more return points (since this way it returns null when organisation is missing)
| { | ||
| if (uri == SonarCloudUri) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(organisation)) { return SonarCloudPrefix + organisation; } |
There was a problem hiding this comment.
newlines missing in if body
There was a problem hiding this comment.
it's to reduce clutter I think the expression is simple enough to read easily.
There was a problem hiding this comment.
I think you're treating a symptom here. This method is hard to read, I completely missed the branch where organization is null (is that even a possibility?)
| var actualConnectionId = testSubject.GetConnectionIdFromUri(null, "something"); | ||
|
|
||
| actualConnectionId.Should().BeNull(); | ||
| } |
There was a problem hiding this comment.
add test that uri == GetUriFromConnectionId(GetConnectionIdFromUri(uri))
|
ugras-ergun-sonarsource
merged commit 5d81b54
into
feature/sloop-rule-meta-data
| if (!string.IsNullOrWhiteSpace(organisation)) | ||
| { | ||
| return SonarCloudPrefix + organisation; | ||
| } |
There was a problem hiding this comment.
Maybe for a refactoring change in the next PR:
This would be easier to understand like this:
return string.IsNullOrWhiteSpace(organization)
? null
: SonarCloudPrefix + organization;
and then removing the least return null
| @@ -33,6 +35,28 @@ internal class ConnectionIdHelper : IConnectionIdHelper | |||
| private const string SonarQubePrefix = "sq|"; | |||
| private static readonly Uri SonarCloudUri = new Uri("https://sonarcloud.io"); | |||
There was a problem hiding this comment.
C#9 target-typed null can be useful here (it's not useful ultimately everywhere, this is a place where it is)
private static readonly Uri SonarCloudUri = new("https://sonarcloud.io");
Fixes #5230