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

new Check: ConstructorsDeclarationGrouping to check the grouping of overloaded constructors #14726

Open
Zopsss opened this issue Mar 27, 2024 · 5 comments
Labels

Comments

@Zopsss
Copy link
Contributor

Zopsss commented Mar 27, 2024

initially discuses at #14415 and #14398
Reason of new check creation is desire to cover Google style request for this - #14273

I have downloaded the latest cli from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

How it works Now:
Checkstyle have a module called OverloadMethodsDeclarationOrder which checks that if the overloaded methods are grouped together or not, if there is another method between overloaded methods then it'll give you an error. Currently, in checkstyle there is no module to check the grouping of overloaded constructors in a class, enum, abstract class, records. This new module will check that the overloaded constructors are grouped together or not. If there is another method or anything else between the constructors ( expect comments ), it will give an error.

How I want check to work

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="ConstructorsDeclarationGrouping"/>
    </module>
</module>

Test.java

$ cat Test.java 
public class Test {
    public Test(){}

    public Test(int a){}

    public void testing() {}

    public Test(int a, int b) {}

}

Expected output
The module should check the grouping of overloaded constructors and give an error if they don't follow the rule. It should give an error something like this:

$ java -jar checkstyle-10.13.0-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] C:\checkstyle testing\Test.java:8:3: All overloaded constructors should be grouped together. Previous overloaded constructor located at line ''4''.[ConstructorsDeclarationGrouping]
Audit done.
Checkstyle ends with 1 errors.

Some other examples showing how the check should work
Example 1: This is fine

public class Example1 {
    Example1 () { }

    Example1 (int a) { }

    Example1 (int a, int b) { }

    void testing () { }
}

Example 2: This is fine as we do not required to have overloaded constructors in a specific order like in ascending order based on the number of parameters or something similar to that. We only care about that if the overloaded constructors are grouped together or not, we don't check their order based on number of parameters or anything.

public class Example2 {
    Example2 (int a, String str) { }

    Example2 () { }

    Example2 (int a) { }
}

Example 3: This will give error, constructors are not grouped together

enum Example3 {
  TEST1, TEST2, TEST3;

  Example3() { }

  Example3(String a) { }

  void testing() { } 

  Example3(int a) { } // violation, all overloaded constructors should be grouped together 
}

Example 4: This is fine

record Example4(int x, int y) {
  Example4() {
    this(0, 0);
  }

  Example4(int x, int y) {
    this.x = x;
    this.y = y;
  }

  Example4(int x) {
    this(x, 0);
  }

  Example4(int x, int y, int z) {
    this(x, y + z);
  }
}

Example 5: This will give error, constructors are not grouped together

record Example5(int x, int y) {

  Example5(int x) {
    this(x, 0);
  }

  Example5() {
    this(0, 0);
  }

  void testing() { } 

  Example5(int x, int y, int z) { // violation, all overloaded constructors should be grouped together
    this(x, y + z);
  }
}

Example 6: This is fine

public abstract class Example6 {

    int a = 19;

    Example6 () {}

    Example6 (String a, String b) {}

    Example6 (int a) {}
}

Example 7: This will give error, constructors are not grouped together

public abstract class Example7 {

    Example7 () {}

    Example7 (String a, String b) {}

    int a = 19;

    Example7 (int a) {} // violation, all overloaded constructors should be grouped together
}

Solution

The module should be able to verify that all overloaded constructors are grouped together in a class, enum, records, etc. The overloaded constructors should be strictly grouped together, if there is a method, variables, etc between them then it will give an error.

@romani
Copy link
Member

romani commented Mar 27, 2024

I am good with proposal.

@rnveach , @nrmancuso , please review to approve.

@nrmancuso
Copy link
Member

nrmancuso commented Mar 28, 2024

@Zopsss please update example 6 to be compilable to make sure we are on the same page.

@Zopsss
Copy link
Contributor Author

Zopsss commented Mar 28, 2024

@Zopsss please update example 6 to be compilable to make sure we are on the same page.

done thanks for pointing this out.


@rnveach can you please review this issue?

@nrmancuso
Copy link
Member

I am good to approve

@nrmancuso nrmancuso assigned rnveach and unassigned nrmancuso Mar 28, 2024
@Zopsss
Copy link
Contributor Author

Zopsss commented Mar 29, 2024

@rnveach

Can you please review this issue? The other maintainers has approved this issue, We need your assurance as the final approval.

And sorry for the ping, weekend is near so I thought the approval may get delayed

@rnveach rnveach removed their assignment Mar 31, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 1, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 4, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 4, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 4, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 6, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 6, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 6, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 6, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 8, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 8, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 8, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 8, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 9, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 9, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 10, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 10, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 14, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 14, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 14, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 14, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 14, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 15, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 17, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 30, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Apr 30, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 3, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 3, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 3, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 4, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 4, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 4, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 4, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 7, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 7, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 7, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 9, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 9, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 9, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 9, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants