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

Improve structure of java classes to make them consistent #538

Closed
cb0s opened this issue Mar 9, 2022 · 4 comments
Closed

Improve structure of java classes to make them consistent #538

cb0s opened this issue Mar 9, 2022 · 4 comments
Labels
👏 help wanted Extra attention is needed 📄 java Pull requests that update Java code ♻️ duplicate This issue or pull request already exists 🌷 enhancement New feature or request
Projects
Milestone

Comments

@cb0s
Copy link
Member

cb0s commented Mar 9, 2022

If you take a look at our Java classes it is visible that not all of them do have the same structure. What I mean by that is the order in which we group our classes.

Imo it should have a strict order to follow. The general rule of thumb should be the following:

  1. public
  2. protected
  3. private

I think that's a good rule because public methods are usually most important to potential users of the API. We can enforce this rule because methods do not have to be declared before being used like in other languages, e.g., C or C++.
This leads to a more clear structure and therefore make the code more clean.

We could also enforce more strict rules for each of the visibilities like:

  1. [visibility] overridden methods
  2. [visibility] methods (custom)
  3. [visibility] constructors
  4. [visibility] getters and setters
  5. [visibility] fields

What do you think about this proposal?

I also prepared an example, I rewrote TelestionVerticle for that matter.

@cb0s cb0s added 🌷 enhancement New feature or request 👏 help wanted Extra attention is needed 📄 java Pull requests that update Java code labels Mar 9, 2022
@cb0s cb0s added this to the v0.10 milestone Mar 9, 2022
@cb0s cb0s added this to Backlog - New Unranked in Telestion via automation Mar 9, 2022
@pklaschka
Copy link
Member

I don't care too much about the order of these things, but here are my two cents:

Beware of creating a false sense of "we're doing enough for the Developer Experience": If you expect users of a library to look at the library's source code, you're not documenting it well enough 😉 . That doesn't mean that no one will ever look at this, but it's essential not to get a sense of "oh, but we structured it nicely, so they can just look at the source code". So overall, the structure within the source code is primarily for maintainers of the repo, not library users (it's good to also think of library users looking at the source code, but we have to make sure that we don't derive a false sense of "that's enough" from that). When we catch ourselves thinking, "ah, they can just look at the source code," ... I don't know, maybe we have to slap each other. But that shouldn't stand 😜

Fields vs. Methods: I'm not too fond of (no matter their visibility) fields coming after methods. I don't know where this stems from (maybe UML diagrams?), but having fields before methods feels wrong. It should be "Fields, Constructors, Methods". How the ordering within that is structured then poses another question.

Don't try to enforce this manually: If you can find a linter/other tools that can enforce this automatically, that's great. But please don't try to implement such rules manually. Without tooling that can automatically detect and fix such stuff, something like this achieves only to make the codebase more hostile to new contributors.

If we define anything like this, we must be precise: What about static members? What about inner classes like Verticle configurations?

@pklaschka
Copy link
Member

Maybe relevant: https://checkstyle.org/styleguides/sun-code-conventions-19990420/CodeConventions.doc2.html#a1852: They also recommend instance variables before methods 😜

If we used checkstyle, that could at least account for the tooling around it 🤔

@cb0s
Copy link
Member Author

cb0s commented Mar 9, 2022

I will look into this after my Bachelors Thesis. Let's put it into Medium priority, I will deal with it in April... :D

@pklaschka pklaschka added the ♻️ duplicate This issue or pull request already exists label Dec 23, 2022
@pklaschka
Copy link
Member

Moving this discussion to #518. Please feel free to re-open (with an updated description) if you disagree.

Considerations for closing this

  1. Linter Integration #518 hopefully solves this (and more)
  2. inactivity

@pklaschka pklaschka closed this as not planned Won't fix, can't repro, duplicate, stale Dec 23, 2022
Telestion automation moved this from Backlog - New Unranked to Sprint Done Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👏 help wanted Extra attention is needed 📄 java Pull requests that update Java code ♻️ duplicate This issue or pull request already exists 🌷 enhancement New feature or request
Projects
Telestion
Sprint Done
Development

No branches or pull requests

2 participants