-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Spatial: code cleanup & javadoc enhancement #2532
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
base: master
Are you sure you want to change the base?
Conversation
public void write(JmeExporter ex) throws IOException { | ||
OutputCapsule capsule = ex.getCapsule(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in 2 previous reviews, changing variable names like this is not warranted.
While I understand your goal of making every write and read method consistent throughout the engine, that is a goal with no benefit and will only add to the maintenance and reviewing burden if we set such a high and non practical standard.
Even if you achieve this level of consistency now, the effort of trying to maintain that consistency and enforcing it upon all future contributors is not worth it. As an open source project, we simply cannot afford to spend time on aiming for such a level of perfection, nor can we afford to hold future contributors to the same standards when they are inevitably contributing for free.
If JME had an infinite budget and infinite manpower, then I'd be glad to say lets go ahead and make code consistent and perfect everywhere and try to maintain that. But resources are limited, and things like using consistent variable naming accross every class' read/write method is a futile effort that is only adding extra lines of code changed to the review load. Aiming for such a level of perfection also adds constraints for future contributors who are now expected to follow our rigorous formatting and variable naming standards. Consistency just isn't important in this context. And while it may be a low risk thing to change variable names like this, it inevitably opens the door to adding onto maintainers workload if we start allowing users to impose their own naming conventions anytime they make changes to a class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider the renaming of variables, especially within methods like write
and read
, to be an integral part of the optimization process outlined in the PR title. Consistent and clear variable naming significantly enhances code readability and maintainability. When variable names like ic
and oc
, are consistently used across related methods throughout the codebase, it reduces cognitive load for developers, makes the code easier to scan, and minimizes potential confusion, particularly in an engine as complex as JME.
Importance of Consistency
While I appreciate your perspective on resource constraints in an open-source project, I respectfully disagree that consistency in this context offers no benefit or that maintaining it is a futile effort. Code consistency, especially in fundamental aspects like variable naming within standard methods, is not about "perfection" but about reducing friction and errors for present and future contributors.
- Reduced Learning Curve: New contributors can onboard faster when they encounter familiar patterns and naming conventions across different parts of the codebase.
- Easier Debugging and Maintenance: When variable names are uniform, it's quicker to trace logic, understand data flow, and identify issues. Inconsistent naming can lead to subtle bugs or misinterpretations.
- Improved Code Quality: Establishing and adhering to clear standards, even for naming, elevates the overall quality and professionalism of the project. It sets a positive precedent for all contributions.
The argument that this adds "extra lines of code changed to the review load" overlooks the long-term benefits in terms of reduced future maintenance burden and increased clarity. The initial investment in consistency pays dividends over time by making the codebase more predictable and easier to navigate for everyone.
Clarifying Expectations for Contributions
Regarding future contributors, setting clear coding standards (including naming conventions) is a common and beneficial practice in almost all well-maintained open-source projects. It doesn't impose undue burden; rather, it provides a clear framework that helps contributors integrate their work seamlessly and maintain the project's integrity. It ensures that the project evolves in a structured and coherent manner, rather than becoming a disparate collection of individual styles.
I believe that these changes, including variable renaming, are a valuable part of improving the codebase's overall health and are well within the scope of code optimization.
Edit:
@JNightRider should know, too, since I reviewed his PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, I'm just trying to make the decision that I believe past core devs and release managers would've made in regards to a mass variable renaming effort, which would be to deny such a change and consider it unnecessary.
Unfortunately there isn't much documentation about engine policy. So I am just doing my best to follow suit with the engine culture and inferred policies I've observed over my past decade working with jme.
There is one contirbution read-me that sgold recently pointed out to me, wnich outline the process for making major changes starting from the planning phase and through the PR and reviewal phase. In hindsight I should have linked you to that before submitting all these PRs, because it does have some good recommendations about what steps to take in order to receive pre-approval before making big changes. And in this case, I think a thread may have been a better place to start to determine if the community as a whole agrees with each aspect of your refactoring efforts.
So if you want to debate this, then I'd suggest starting a thread titled something like "renaming variables consistently accross multiple jme classes" so that this can be discussed and potentially approved. My suspicion is that certain core devs will disagree (hence my position thus far) but I also am willing to change my policy on this if other core devs or contributors turn out to feel otherwise.
This PR refactors the
Spatial
class to enhance code clarity, maintainability, and documentation.Key changes include:
propagateRefreshFlag
)These updates aim to make the codebase more understandable for contributors and maintainers, while preserving all existing functionality.