DuplicateBranches should not warn if there are different comments
- Dominant language
- Java
- Stars
- 7.2k
- Forks
- 820
- Avg merge
- 5h 9m
- Merged PRs (30d)
- 50
Description
In some cases we have code where the new `DuplicateBranches` check triggers but the code was intentional because the same thing needs to be done for different reasons, and there are comments explaining this. In a few more cases the code is unfinished and the duplication is only temporary, marked with a TODO, and to be replaced by different code later. In these cases it would be possible to remove the duplication and explain the situation in a comment, but I think that actually having the branching code makes it more clear and less error prone compared to having to write down what each branch would be in a textual comment. Furthermore, the branching code benefits from refactorings being applied automatically, whereas comments need to be updated manually.
### Examples:
#### Preferred:
```java
public static void main(String[] args) {
if (args.length == 0) {
// nothing we can do here
return;
} else {
// TODO: add proper argument handling here
return;
}
}
```
#### Less optimal:
```java
public static void main(String[] args) {
// TODO we cannot handle empty args, but need to add proper argument handling args.length > 0
return;
}
```
#### Preferred:
```java
public static void main(String[] args) {
if (args.length == 0) {
// nothing we can do here
return;
} else if (args.length == 1) {
// some other reason why the following is intended
return;
} else {
...
}
}
```
#### Less optimal:
```java
public static void main(String[] args) {
if (args.length < 0) {
// either args is empty, then there is nothing we can do here,
// or args has one element, then some other reason why the following is intended
return;
} else {
...
}
}
```
In both cases the actual code can be more precise than a textual description (especially if the conditions get more complex) and would benefit from refactorings such as renamings of `args`.
Thus I would suggest to change `DuplicateBranches` such that it does not warn about cases where (different) comments exist in each of the branches. `@SuppressWarnings` is unfortunately not a good workaround because one would have to label the whole method with it.
Contributor guide
Assessment
This issue has not been assessed yet.