Order of members in Types (Class, interface)

Development
#1

Hi devs,

I’m updating the IntelliJ code style rules. My original goal was to reformat all files to apply https://forum.xwiki.org/t/checkstyle-adjustments/6642/7?u=vmassol and add it to our checkstyle rule, as we decided.

However, this means ensuring we’re also ok on some other rules since the global reformatting of IntelliJ IDEA will also reformat other stuff. I’ve tested it and I think the main point to agree is about the order of fields, methods, etc.

What I’ve defined and thus proposing is the following:

Screenshot 2020-05-25 at 13.52.26
Screenshot 2020-05-25 at 13.52.261344×1760 242 KB

With “visibility” meaning this order:

Screenshot 2020-05-25 at 13.52.36
Screenshot 2020-05-25 at 13.52.361642×436 38.7 KB

WDYT? I’m open to other proposals/adjustments ofc.

Thanks

PS: Does Eclipse also support this? I guess so.

#2

This is what we already have in checkstyle AFAICS. At least I get a checkstyle error if I don’t order fields like this.

I assume you are only talking about fields since it’s a big -1 for me for methods.

It’s not part of the source formater but there is a dedicated “Sort Members” tool which supports this (which I never used, just discovered it).

#3

Thanks for you opinion but could you elaborate a bit why?

#4

Well I could ask you the same thing since you are not giving any argument to introduce this new rule :slight_smile:

I simply don’t see any benefit in enforcing an order for methods and I really doubt you will find many classes where what you are proposing is done. Also in most cases I much prefer grouping them by subject (i.e. private sub methods next to the public methods which are using them).

#5

Fair enough.

So the reason is the same as the reason we have an order for the fields:

  • Find what you’re expecting in a given location
  • Be able to reformat to a stable layout, i.e avoid dev #1 formatting and moving methods around and then having dev #2 also do that and move the methods elsewhere. For ex you say you prefer to; group methods together (private methods coming just after public/protected methods using them). Personally I don’t do that. I put private methods at the end because I consider that for someone reading the files it’s more important to more visibly see the public methods. In any case my point is not that my strategy is better than yours or not, it’s just to have one so that we don’t move methods arounds and undo the work of others, and we can reformat.

I could very well have applied my formatting without asking and that’s valid since there’s no rule about it. Is that ok with you? Or do you consider that once a dev created a method in a given location it’s forbidden to move it?

Thanks

#6

I agree with Thomas.
Another argument is that most of the time we are using modern IDEs where navigating in the code is really easy and does not depend of the actual ordering of the declaration of the methods in the code.

Forcing an order on existing code will probably make a mess of the git history, especially since diff algorithms are bad at identifying moved blocks.

I could be convinced that enforcing an order on newly introduced code might have some benefit, but probably not on existing one.

#7

And my point is that there is no strategy that is obviously better enough to enforce it here.

Not really sure what is your point. Among many others one reason to not do pointless things like this is that it makes cherry-picking a nightmare.

#8

@mleduc Same comment as for Thomas. Are you ok that I run my formatter on it or is that forbidden? If the answer is true then it’s all fine, we can close the topic, and even remove the field order rule if you want.

#9

Yet we do that all the time (moving methods around).

#10

Remember that Thomas does that regularly too :slight_smile: He broke the formatting for almost every single component test class (and every week I fix them manually one by one). What I mean is that the topic is not just for methods but more general (i.e. all styles that are not enforced by checkstyle).

EDIT: I mentioned Thomas here because whenever I see my carefully crafted formatting broken, I cannot help cursing Thomas. And I don’t think that’s great. Now I don’t think he did it on purpose, but the same can be said for javadoc parameter alignment, or plenty of others things that we haven’t configured in checkstyle.

Let me say it differently: there’s a reason checkstyle exists and why we use it. The question is: why not continue fixing styles?

#11

BTW we just did it recently with https://forum.xwiki.org/t/checkstyle-adjustments/6642/8 so I guess here is more about method order than fixing other checkstyle rules. Personally I don’t see method order different than field order.

#12

Do you agree at least to have the constructor first? :slight_smile:

#13

FTR, style discussions are always fun. I don’t care that much about this topic but I’m curious to understand our rationales and to discuss the limits of what we’re prepared to do or not do.

#14

BTW @tmortagne, about the following issue, did you open an issue on eclipse’s tracker to ask to support it? Works fine with IntelliJ’s idea reformatting for ex, and I don’t feel that it’s right to litter our test code with eclipse-specific reformatting comments. Would be much nicer to have Eclipse not touch it.

// @formatter:off
@ComponentList({
    InfinispanCacheFactory.class,
    DefaultCacheManager.class,
    DefaultCacheFactory.class,
    DefaultCacheManagerConfiguration.class
})
// @formatter:on

BTW the initial reason I started this thread was because I wanted to implement https://forum.xwiki.org/t/checkstyle-adjustments/6642/8 but IDEA doesn’t allow to apply only part of the formatting rules (it’s all or nothing). I believe this was the same for you for Eclipse above and the reason why you applied the wrong formatting instead of skipping it. Right now I have to change the IDEA formatting completely, apply and then put it back (so that I can write new code following it). And do this every time, I reformat existing code, It’s a pain.

#15

I do this also.

I don’t understand this argument. When you’re searching for a method you might not know whether it’s public or private so you wouldn’t know where to look. Also, when you read the code and see a method called you don’t know either if it’s public or private, until you get to its definition. And it’s not like IDEs mark on the scroll bar the place where public / private methods start. You’d have to scroll to find the method definition, which is harder than using the code navigation provided by all IDEs (Ctrl+Click on method name, class outline panel, etc.). So I don’t see how sorting the methods by access type helps you find the method definition.

For me it makes sense to sort the class fields because since they take little vertical space you can scan the list quickly, most of the time, and thus the sort can help you understand the “structure” of the class (what data is static, what data is public, etc.). I don’t see the benefit of sorting the methods because they take a lot of vertical space so you can’t easily scan the entire list of methods directly from the source file. You need the class outline for this, which makes the method order less important for scanning the source code (because the class outline doesn’t have to respect the order from the source file), so I’d prefer to let the developers organize the methods the way they want.

If you mean automatic formatting then the point is to not configure the automated formatter to move methods around. Maybe I didn’t understood, but ATM the automatic formatter doesn’t move methods around, at least in Eclipse, and you propose to configure the automatic formatter to do so.

No, because there’s no rule to sort the methods either, so I would have asked you to move the methods back. My point is that you shouldn’t re-format existing code unless there is a rule for it or you have a good reason.

Formatting long lines of code is a bit different for me. Personally I try to avoid re-formatting the code I don’t change (by selecting and formatting only the code I change). But sometimes I do re-format the long lines I change, using the automatic formatter. If there is a better way to configure how long lines are formatted then I’m OK to apply that, i.e. if you can configure the automatic line formatter to have an output that is closer to the way you manually format long lines then I’m OK to use that style. But I don’t think that formatting long lines is the same as moving methods around.

No, it’s not OK with me. I wouldn’t move methods around unless I have a good reason.

Yes.

Thanks,
Marius

#16

We already have this checkstyle rule.

1 Like
#17

Closing this thread since we decided we don’t want that. Thanks for the feedbacks!