rewrite-static-analysis icon indicating copy to clipboard operation
rewrite-static-analysis copied to clipboard

721: Recipe to move fields to the top of class definition

Open dsgrieve opened this issue 3 months ago • 1 comments

What's changed?

Added MoveFieldsToTopOfClass recipe and tests. This recipe reorders class members so that all field declarations appear before any method declarations constructors, or other class members. This improves code organization and readability by grouping field declarations together at the top of the class. Comments associated with fields are preserved during the reordering.

What's your motivation?

  • Fixes #721

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

This PR adds the recipe to static-analysis.yml. I'm not sure if that should be done in a separate PR, or not done at all.

Checklist

  • [X] I've added unit tests to cover both positive and negative cases
  • [X] I've read and applied the recipe conventions and best practices
  • [X] I've used the IntelliJ IDEA auto-formatter on affected files

dsgrieve avatar Sep 09 '25 18:09 dsgrieve

The current issue is keeping a static intializer with the variable it's initializing. For instance, this simple test case will fail.

    @Test
    void staticInitializerStaysWithDeclaration() {
        rewriteRun(
          //language=java
          java(
            """
              class WithStaticInitializer {
                  public void method() {
                      System.out.println("method");
                  }

                  private static final String field1;
                  static {
                      field1 = "value1";
                  }

                  public WithStaticInitializer() {
                      // constructor
                  }
              }
              """,
            """
              class WithStaticInitializer {

                  private static final String field1;
                  static {
                      field1 = "value1";
                  }
                  public void method() {
                      System.out.println("method");
                  }

                  public WithStaticInitializer() {
                      // constructor
                  }
              }
              """
          )
        );
    }

In addition, I would expect a static initializer block that initializes more than one variable to appear after the last variable declared (of those being initialized). I would also expect that other static blocks that aren't initializing a variable (doing something you want done when the class is first loaded) to come after all the variable declarations. None of these are being handled.

Also, as @Jenson3210 points out, this could be a reorder statements recipe - which it is tending toward anyway. I'll add that how the statements are ordered should be a Style (even for the field decls, the ordering for which follows Oracle code conventions

Lastly, this recipe in its current state messes up the formatting. But, as discussed with @sambsnyd, trying to maintain spacing seems overreach for this recipe.

I feel, however, that without handling statement reordering and formatting this recipe in its current incantation just makes a mess of things. In short, moving the fields to the top is fairly trivial. Doing it correctly, however, is a bit tricky.

dsgrieve avatar Sep 22 '25 00:09 dsgrieve