Skip to content

Conversation

@yaRnMcDonuts
Copy link
Member

@yaRnMcDonuts yaRnMcDonuts commented Jun 11, 2025

I accidentally approved and merged this PR (#2474) without realizing that the static keyword had been removed from a set of String vars

So this PR re-adds the static keyword back.

My apologies for overlooking this before merging the PR.

I accidentally approved and merged this PR  (#2474) without realizing that the static keyword had been removed from a set of String vars, and this mistake breaks any apps that are using JoyStickButton for controller input. Despite reviewing this fairly small PR twice, I unfortunately still didn't see this.

So this PR re-adds the static keyword back. I will merge this in a few hours to ensure that the master branch isn't broken for too long. 

My apologies for the overlooking this mistake in my review, I should also be more hesitant to merge PRs in cases like that where I was the only reviewer, but unfortunately there is a lack of other active reviewers currently, so I just have to do my best I suppose. 

Thankfully there shouldn't have been any any harm done since this was only broken master for a day and I managed to catch it almost immediately after merging.
@yaRnMcDonuts yaRnMcDonuts added this to the v3.9.0 milestone Jun 11, 2025
@capdevon
Copy link
Contributor

capdevon commented Jun 11, 2025

All variables declared in a Java interface are public, static, and final by default.
You don't need to explicitly write these modifiers; the compiler adds them automatically.
So there is no mistake in my PR.

The test class TestJoystick compiles correctly even without the static modifier.

@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Jun 11, 2025

Oh, I forgot that it was an interface during my final review, since it was late and I was looking at only the changed lines of code after I merged it.

And I also just assumed since Paul wrote this class that it was right the way it was originally written. But yes you are correct it should be functional and compile either way. Just a difference in whether a verbose or minimal style is preferred.

With that said, I do actually think the static keyword should probably not have been removed in your PR in the first place, just because the benefit is so negligible, and every changed line of code adds extra work for reviewing and managing PRs.

And whether or not "static" should be left for clarity/consistency is still arguable. Although to be honest, I personally don't have interests about debating over negligible code changes like this, such a minor code change is rarely worth the time since things like this come down to personal preference whether someone prefers verbose code or condensed code. I personally prefer verbose code, so I actually disagree with some code changes in a few PRs I've passed this year, but I also don't have strong preferences or think its worth spending time thinking about too much, so I just let the author of the PR go with whatever style and approach they want when adding something new as long as the new thing is worth it and the code is clean to an acceptable standard. And if its an existing class I usually lean towards just following the style the original author went with for simplicity in order to minimize changes to code that is already tested and stable)

I do still greatly appreciate all of your efforts with your recent PRs cleaning up and refactoring code for organization, and mostly all of your changes refactoring old classes so far have been great improvements. For example, I agree 100% with all of your formatting improvements with fixing the spacing and indentation and other minor things like that which you have a good eye for. That type of code cleanup and refactoring has no chance of breaking anything and doesn't change code in a way that adds on to the workload for reviewers.

But I'd recommend trying to minimize refactoring existing code whenever the benefits are very negligible, especially in cases that are arguable as to whether its an objective improvement or not. Or if you do have intentions on reorganizing a class in a way that is purely stylistic or may be arguable to someone else with differing preferences, then it would be best to split into another PR where that is the focus.

@capdevon
Copy link
Contributor

As mentioned on the forum, collaborating together, the next release will be full of new features, substantial improvements, and optimized existing code. My current efforts are concentrated on refining the most resource-intensive classes to enhance their efficiency in update and render cycles.

My preference is for a more concise style when the code's intent is clear, and I adopt a more detailed approach for complex algorithms. Unfortunately, the jme code base is full of repetitions, incorrect formatting, incomplete, absent or wrong javadocs. When proposing a PR it is easy to get carried away and try to optimize things. I will try to create more focused PRs in the future to ease the review process.

I'm certainly willing to revert the changes to the original style in this PR if that's preferred. If we go that route, though, could we revisit the PR title? Its current wording feels a bit accusatory and doesn't accurately reflect the situation.

Thanks

@yaRnMcDonuts yaRnMcDonuts changed the title Fix mistake in JoystickButton from PR 2474 Revert Static keyword in JoystickButton Jun 11, 2025
@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Jun 11, 2025

could we revisit the PR title? Its current wording feels a bit accusatory and doesn't accurately reflect the situation.

Thanks

I've renamed the title. At the time I just thought it was a potential mistake, so I named the title in a descriptive way to explain what I believed to be the case.

The intent was not to be accusatory in any negative way with the title, just objective and accurate based on what I thought at the time. And even though I was indeed wrong to call it a mistake, it's also no big deal if it had actually been a mistake. Mistakes are unavoidable (otherwise all code would be bug free) and that's what review and testing is for, so there is no reason to feel accused of anything negative in cases where a mistake is actually made in a PR. And if anything, it's my fault more than anything for thinking it was a breaking change in the first place and then also merging it before I had my incorrect realization.

And that's perfectly fine to apply your style to your PRs especially when adding new things or when you're already refactoring existing code for functionality reasons. But as mentioned, it's important to remember that many aspects of code style are opinion, so I think its also wise to consider when changing something like that is worth it, just because more will inevitably lead to making the review load larger, and ultimately its always best to leave as much stable tested code alone as possible just for the sake of minimizing risk. So negligible styling changes will only slow things down for little benefit. If there were more reviewers and active maintainers merging things, then I'd say its no big deal and to open as many PRs discussing the best style and approach as you'd like. But since I'm not particularly keen on all the best standards in the first place, those type of PRs are more likely to slow me down or cause me confusion (like it did in this case since I'm not used to minimizing code and forgot it was even possible in cases like this, so I ended up mistaking a styling change for a functionality change)


Without getting too deep into a discussion of verbose vs minimal (and as someone who mixes both approaches to make my classes concise while still verbose and easily understandable at quick glance; in my own opinion at least), I'll explain briefly why I'd leave this static when considering the choice for the jme codebase:

In this case, the extra verboseness from including the word static is so minimal that the removal of it has very little benefit to improving readability or organization. The line of code declaring these Strings is already short enough that it won't span far enough across a standard size monitor to be problematic or cause side scrolling, and there are other lines of code longer than this one in the class already anyways so reducing it by 6 characters has very little benefit.

And the benefits of being verbose and including the word static is that any user who looks at a single line of code for one of these String variables will immediately know that it is intended to be used in static context without a doubt. If static not there, they will be required to look further into the source code to determine that information.. The difference is minimal, but in this case the verboseness is marginally better and improves the understandability of the class. And I believe that having a core engine class be verbose and easy to understand is more important than reducing code by a few characters or lines in cases like this.

@capdevon
Copy link
Contributor

@yaRnMcDonuts It's ok with me. You can merge this PR at any time.

@yaRnMcDonuts yaRnMcDonuts merged commit c60cb56 into master Jun 14, 2025
16 checks passed
@yaRnMcDonuts yaRnMcDonuts deleted the yaRnMcDonuts-patch-2 branch June 14, 2025 08:11
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.

3 participants