Skip to content

Conversation

JooHyukKim
Copy link
Member

Initiates a long journey of ... FasterXML/jackson-databind#4156

If we start this, everyone's advised to link any discussions regarding coding-style, or conventiuon back to this style-guide

@pjfanning
Copy link
Member

I code mainly in Scala and that language has tools to validate and even reformat code based on agreed styles. If you can't automate it, I would suggest it is more time consuming than it's worth to argue about indents in PRs. Java community might have tools but may not.

@pjfanning
Copy link
Member

https://code.revelc.net/formatter-maven-plugin/ is an example of a tool that could be looked at.

@JooHyukKim
Copy link
Member Author

If you can't automate it, I would suggest it is more time consuming than it's worth to argue about indents in PRs

I concur. The intention was that this new "Jackson Coding Style Guide" will serve both as a guideline, but also as specification for such automated formatter

There is also https://github.com/google/google-java-format

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 10, 2023

Quick note: I have not have good experience with auto-formatters so I am generally -1 for using them in fully automated mode. Some aspects could be useful (automatic trimming of trailing spaces, tabs to spaces), but almost universally there are stylistic exceptions that cannot be automated.
At work we use Google auto-formatter and that leads to about ugliest code imaginable due to rigid line-length enforcement. I assume there are variations, but fundamentally lots of time can be spent on trying to fine-tune formatters, not unlike various annotations that need to be added for things like FindBugs etc to weed out false alarms.

So I don't have much expectation that formatters would be valuable addition.

But I do think documenting style itself, important parts, is a good thing: most contributors follow existing style anyway, but spelling out intent -- and especially CURRENT thinking (some code may follow older style; style evolves along with code).

But I also agree that what we do not want to do is to spend lots of (or, any... ) additional time worrying about style-compliancy. Use of auto-formatters is definitely one way to do that; the other is to do things in eventually consistent way.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 10, 2023

Looks good so far!

One other thing we could mention is the use of final keyword:

  1. Use of final fields (assign in constructor) is strongly encouraged wherever possible: immutability is a goal
  2. Use of final for method parameters is allowed but not necessarily encouraged
  3. Use of final for local variables is encouraged (where applicable)

Yet another is maximum line length. This is tricky, because strict adherence can lead to really awkward lines, coupled with indentation. So I think we should probably just suggest lines not to exceed... I don't know. Looks like 90 characters is width of lines I see (80 or 76 is traditionally recommended for all TTYs).
Maybe we should recommend that line length not exceed 100 characters.

For Javadocs we could/should recommend strict adherence. Maybe starting with max 100 characters.

Which leads to question of what to mandate about Javadocs more generally.

@cowtowncoder
Copy link
Member

Looks good @JooHyukKim. Aside from question of "static import" ordering, one other existing rule I realized related to test class naming:

  • Earlier, naming of "TestXxx" was used.
  • Current preference is instead using "XxxTest" (and I have slowly refactored names over time, but this is bit tricky with 2.x/3.0 discrepancy)

It is worth noting that one of above is (was?) actually required by JUnit (wrt Maven pom settings) to include test classes: classes that don't start or end with Test are excluded from test runs.

Similarly there is specifically named src/test/java/**/failing directory/package, from which tests are not automatically run: these contain JUnit tests that fail due to reported bugs, and are intended for manual runs, to reproduce issues.

@JooHyukKim
Copy link
Member Author

Looks good @JooHyukKim. Aside from question of "static import" ordering, one other existing rule I realized related to test class naming:

Added Test section, PTAL? @cowtowncoder

@JooHyukKim
Copy link
Member Author

Looks good @JooHyukKim. Aside from question of "static import" ordering, one other existing rule I realized related to test class naming:

Added Test section, PTAL? @cowtowncoder

After the "test" part is reviewed, shall we conclude this? As we may run into more guidelines in the future, this guide was supposed to serve as simple starting point wrt layout and such.

@cowtowncoder
Copy link
Member

Looks good @JooHyukKim. Aside from question of "static import" ordering, one other existing rule I realized related to test class naming:

Added Test section, PTAL? @cowtowncoder

After the "test" part is reviewed, shall we conclude this? As we may run into more guidelines in the future, this guide was supposed to serve as simple starting point wrt layout and such.

Yes, I think getting a simple agreed-upon structure makes sense; easy to add/modify as necessary.

Will read through once more now.

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

Successfully merging this pull request may close these issues.

4 participants