auto-arranging code to satisfy CheckStyle DeclarationOrder

[keturn]

When CheckStyle complains “Variable access definition in wrong order [DeclarationOrder],” it's not clear how to satisfy it, and IntelliJ's Reformat Code action is not always sufficient.

Update either the IntelliJ code style definition or the CheckStyle rules so the two match.

For IntelliJ IDEA, see https://www.jetbrains.com/help/idea/2021.2/code-style-java.html#arrangement_tab

See also: discussion thread

How to reproduce

This is a sample of a class with a bunch of field types, extracted from ModuleTestingEnvironment:

class Scratch {
    @Deprecated
    public static final long DEFAULT_TIMEOUT = 30000;

    public static final long DEFAULT_SAFETY_TIMEOUT = 60000;
    private static final Logger logger = LoggerFactory.getLogger(Scratch.class);
    private final Set<String> dependencies = Sets.newHashSet("engine");
    private String worldGeneratorUri = "moduletestingenvironment:dummy";
    private boolean doneLoading;

    PathManager pathManager;
    PathManagerProvider.Cleaner pathManagerCleaner;


    public static void main(String[] args) { }
}

CheckStyle complains about where pathManager is placed here, as a package-private non-static field. Reformat Code does move those lines, but CheckStyle is still not content with the result.

[keturn]

CheckStyle DeclarationOrder

[jdrueckert]

According to the Checkstyle Documentation for the DeclarationOrderCheck, they reference the "Code Conventions for the Java Programming Language" (Oracle version) which states that the parts of a class or interface declaration should appear in the following order:

1. Class (static) variables. First the public class variables, then protected, then package level (no access modifier), and then private.
2. Instance variables. First the public class variables, then protected, then package level (no access modifier), and then private.
3. Constructors
4. Methods

The Google Java Style Guide for instance states the following:

"The order you choose for the members and initializers of your class can have a great effect on learnability. However, there's no single correct recipe for how to do it; different classes may order their contents in different ways.
What is important is that each class uses some logical order, which its maintainer could explain if asked. For example, new methods are not just habitually added to the end of the class, as that would yield "chronological by date added" ordering, which is not a logical ordering."

@keturn From what I've seen (please correct me if you've seen this differently), I don't think we have a lot of possibilities to configure this differently on checkstyle side. We could try to configure the code style on intellij-level via the .editorconfig with rules based on the arrangement rules you linked. Do you have any experience with that or any concerns / counter-arguments / different ideas?

[keturn]

Thanks for finding the reference for the ordering!

I have nothing more to add at this time; this particular aspect of configuration is not one I've worked with.

[jdrueckert]

Okay, then as long as we can agree on that it makes more sense to adjust the intellij side rather than the checkstyle side, I'll try to figure out how we can do exactly that 🙂

[jdrueckert]

Using your example above, when I do a "Reformat Code" in Intellij, it moves the pathManager and pathManagerCleaner between dependencies and worldGeneratorUri. Interestingly, when I do a second "Reformat Code", it moves the two package-private fields behind the private final ones.
When I move everything back to the way it is in your example and do another "Reformat Code" it directly finds the right place 🤔

For reference my Editor > Code Style > Java > Arrangement configuration looks as follows:

Side note: Aren't rules 16 and 18 the same? I was under the impression that no access modifier defaulted to package private 🤔

Based on this, I would expect the following order, which is the one intellij eventually finds and I think should also match CheckStyle's requirements. So maybe we don't have an issue after all except for some IntelliJ weirdness? 🙈

	// CS class (static) variables in order: public, protected, package-private, private
	// IJ static final in order: public, protected, package-private, private
    public static final long DEFAULT_TIMEOUT = 30000;
    public static final long DEFAULT_SAFETY_TIMEOUT = 60000;
    private static final Logger logger = LoggerFactory.getLogger(Scratch.class);

	// IJ static but not final in order: public, protected, package-private, private
	// IJ static initializer

	// CS instance variables in order: public, protected, package-private, private
	// IJ final but not static in order: public, protected, package-private, private
    private final Set<String> dependencies = Sets.newHashSet("engine");
    private final String worldGeneratorUri = "moduletestingenvironment:dummy";

	// IJ neither static nor final in order: public, protected, package-private, private
    PathManager pathManager;
    PathManagerProvider.Cleaner pathManagerCleaner;
    private boolean doneLoading;

	// IJ non-static initializer

	// CS constructors
	// IJ constructor

	// CS methods
	// IJ static methods
    public static void main(String[] args) {
    }

[jdrueckert]

Actually I just noted that IntelliJ tricked a final into there that was not present before 🤯

Your example:

private final Set<String> dependencies = Sets.newHashSet("engine");
private String worldGeneratorUri = "moduletestingenvironment:dummy";
private boolean doneLoading;

PathManager pathManager;
PathManagerProvider.Cleaner pathManagerCleaner;

First execution of "Reformat Code":

private final Set<String> dependencies = Sets.newHashSet("engine");
PathManager pathManager;
PathManagerProvider.Cleaner pathManagerCleaner;
private String worldGeneratorUri = "moduletestingenvironment:dummy";
private boolean doneLoading;

which is still correct but immediately followed by IntelliJ adding the final keyword for worldGeneratorUri:

private final Set<String> dependencies = Sets.newHashSet("engine");
PathManager pathManager;
PathManagerProvider.Cleaner pathManagerCleaner;
private final String worldGeneratorUri = "moduletestingenvironment:dummy";
private boolean doneLoading;

Which is why the second execution of "Reformat Code" has something new to reformat into:

private final Set<String> dependencies = Sets.newHashSet("engine");
private final String worldGeneratorUri = "moduletestingenvironment:dummy";
PathManager pathManager;
PathManagerProvider.Cleaner pathManagerCleaner;
private boolean doneLoading;

Which should be correct for both IntelliJ Code Style configuration and CheckStyle 🤔

[keturn]

class C {
    private final Set<String> a;
    public Set<String> b;
}

is well-ordered according to IJ, because final instance variables go first.

CheckStyle complains because that is a private placed before the public.