Skip to content
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

if with identical then and else branches #2537

Closed
chavacava opened this issue Sep 24, 2022 · 5 comments · Fixed by #2545
Closed

if with identical then and else branches #2537

chavacava opened this issue Sep 24, 2022 · 5 comments · Fixed by #2545
Assignees
Labels
bug Something isn't working needs discussion Large change that needs review from community/maintainers

Comments

@chavacava
Copy link
Contributor

In the following code, both control branches do the same (i.e. owner = strings.Split(uri.Path, "/")[1])

} else if strings.Contains(uri.Host, "dev.azure.com") {
owner = strings.Split(uri.Path, "/")[1]
} else {
owner = strings.Split(uri.Path, "/")[1] // to support owner for self hosted
}

Found by revive (rule identical-branches)

@jamengual
Copy link
Contributor

yes but one will match a hostname and the other will not.

how do you propose to make it better?

@jamengual jamengual added bug Something isn't working needs discussion Large change that needs review from community/maintainers waiting-on-response Waiting for a response from the user labels Sep 24, 2022
@chavacava
Copy link
Contributor Author

chavacava commented Sep 24, 2022

yes but one will match a hostname and the other will not.

As is written, the value of owner does not depend on the condition (strings.Contains(uri.Host, "dev.azure.com"))

how do you propose to make it better?

In general, if both branches do the same, the full if-statement can be safely replaced with any of its branches, therefore

		if strings.Contains(uri.Host, "visualstudio.com") {
			owner = strings.Split(uri.Host, ".")[0]
		} else if strings.Contains(uri.Host, "dev.azure.com") {
			owner = strings.Split(uri.Path, "/")[1]
		} else {
			owner = strings.Split(uri.Path, "/")[1] // to support owner for self hosted
		}

can be rewritten as

		if strings.Contains(uri.Host, "visualstudio.com") {
			owner = strings.Split(uri.Host, ".")[0]
		} else {
			owner = strings.Split(uri.Path, "/")[1]
		} 

Warn: accessing the second element of the result of a strings.Split (as in strings.Split(uri.Path, "/")[1]) can lead to a runtime error if the result of the split is a slice shorter than 2

@jamengual
Copy link
Contributor

I C what you mean, yes , that could be improved, are you willing to send a PR over?

@chavacava
Copy link
Contributor Author

Yes, sure, you can assign the issue to me

@jamengual jamengual removed the waiting-on-response Waiting for a response from the user label Sep 24, 2022
@jamengual
Copy link
Contributor

I can't assign to you, it does not let me because you are not in the maintainers list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs discussion Large change that needs review from community/maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants