Spring Cleaning 2020 is here!

So, you might be confused about the timing. Yes, I’ve been in a backlog due to a (still unsolved) tooling question around this and then forgot to push the idea further until just recently we’ve been coming up with things that would want to build upon this.

What is it about?

This is a community effort to sieve through the code base to find hidden issues and bugs while increasing the overall code quality, reliability and others, before we start focusing on new features again.

When?

Starting “now” - Ongoing Process

How?

For this, let’s start with an example:
Bring your engine on commit hash a96eb086b0c1071116b9e6999e4200b4b2011a9a. This is the latest as of writing, but the issue we’re searching for might be solved at the time of reading.

Modify gradle.properties to enableSpotBugs=true (DO NOT COMMIT THIS)

Run ./gradlew spotbugsMain or gradlew.bat spotbugsMain (windows) or use your IDE for that.
This will now check the whole sourcecode for dodgy or faulty code and produce a big report for every of our modules/artifacts/subprojects.

For the sake of demonstration, we’ll look into the report jme3-desktop/build/reports/spotbugs/main.html, but feel free to look into any module and browse a bit.

Under correctness it should say There is an apparent infinite loop in jme3tools.navigation.StringUtil.padNumZero(float, int, int).
Click the entry to expand it.

Did you spot it? Is this a false positive?
THIS is the most important step. Most of those reports are either style related or plain false positives, it takes a human intelligent brain to really understand code.

Now I’m going to post the snippet here:

dpLoc = numStr.indexOf(".");
int length = numStr.substring(dpLoc).length();
while (length < decimalPlaces) {
   numStr += "0";
}

I guess the original author wanted to include the length calculation into the while-body or should’ve just used a for loop.
Here we’re at the second most important part. Trying to understand what the reason was and how to go on fixing the issue, sometimes the easiest fix is not the best. For instance the thing I ask myself when seeing this method is:

  • Couldn’t this all be done with string.format?
  • Who did even know about this methods/class/packet?
  • Does it even belong to jme3-desktop?

So in this case a small hub topic or issue would be awesome instead of just throwing a PR at us, if you are uncertain, just ask!
After all humans checking critical/dodgy code that might’ve not blown up at runtime “yet” is exactly what we want to achieve, so please just be verbose.

Note: The most critical warnings there are unused variables. Just plain deleting them is wrong in 99% of the cases. The author did want to do something with them, maybe he referenced the wrong variable? maybe it got shadowed? This is where the thinking begins.

The Process

So unfortunately I cannot award you with virtual monkey coins or a cool badge, but your stats on the github repo will increase and you and we all will gain more confidence in the codebase, so thanks for your time and work!

If you’ve taken a look at something, please report back. If it was a false positive, maybe drop us a line under this topic, so others can skip it. That also means check back this post before doing too much duplicate work.
If what you found is valid (and does not require discussion), feel free to create a PR and assign @MeFisto94 as a reviewer on Github (but the other team members are free to do reviews as well, if their time allows), we’ll get through it.

Do note that your PR might be checked by Codacy, we’re still in the process of setting it up properly, so don’t get irritated by it’s judgement on your PR. If your code didn’t introduce something, we’ll ignore it and integrate your PR. Maybe what it found might also be interesting to do as Spring Cleaning.
IF Codacy finds new issues in your code (e.g. formatting, unused imports/variables), that’d be something you need to address, of course. But we can talk about that on github.

Thanks for the long read, and thanks for participating!

Edit1: Another good source is jmonkeyengine - Codacy - Issues, at least for the throw new Exception cases, which just validate a parameter and thus should be an IllegalArgumentException.

Same applies for jmonkeyengine - Codacy - Issues

Edit 2: I’ve decided that some github issues might help in guidance and coordination, so I’ve opened one and plan to add more, just check the Label Spring Cleaning.

9 Likes

2 link views in 1.5 months.
Bump, maybe someone wants to look at it during their holidays, so we can start with full force into the new year.

3 Likes

For me personally I dont feel I have the time or the competency to assist you. I am curious why NullPointerException should be avoided (according to 2. link)

Code should never throw NullPointerException because that’s Java’s job. If you are going to bother to throw an exception then throw a better one.

As a developer, you should be confident that a NullPointerException is actually because of a null pointer dereferncing and not because some drunk developer decided to throw one randomly for whatever reason.

1 Like

Do I understand it right, to say that if a developer has a need to do a nullpointer exception, they should instead go back in their chain of command and change the design so that the null pointer never exists?

Give me an example of where you think you should write in your very own .java file:

throw new NullPointerException();
1 Like

I can’t. Throwing them doesn’t make any sense.

Thanks

I think in almost all of the cases in the above link, they should have been IllegalArgumentException.

I think in one or two cases it so happens that the empty-message null pointer exception was going to happen one or two lines down anyway.

Somewhere there is a write up I did on the procedure for replacing these intelligently… maybe on a problem report, I don’t remember.

2 Likes