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

DuplicateBranches should not warn if there are different comments #4734

Open
PhilippWendler opened this issue Dec 20, 2024 · 0 comments
Open

Comments

@PhilippWendler
Copy link

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:

  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:

  public static void main(String[] args) {
    // TODO we cannot handle empty args, but need to add proper argument handling args.length > 0
    return;
  }

Preferred:

  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:

  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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant