jmonkeyengine: Manually throwing NullPointerExceptions

So I figured we might help Spring Cleaning by having some major Issues outline here under the Spring Cleaning Label. This issue is about cases where a NPE is thrown, see here for the cases.

According to @pspeed42 NPEs should never be thrown by usercode, instead a more specific Exception, such as an IllegalArgumentException should be used. Also starting from Java 14, NPEs get more detailed, unless when throwing it manually.

The only thing we @jMonkeyEngine/core need to decide on is to how we would want to approach such cases:

  • Remove the Exception and let a NPE happen further down the code naturally
  • Manually throw an IllegalArgumentException
  • Use assert foo != null;, that way the runtime overhead of null checking is not there [though one can probably argue that the jvm does so anyway]
  • Objects.requireNonNull

Personally I’d vote for the approaches in the inverse order, so bottom up would be my priority. Mostly because requireNonNull seems to be the Java7+ canonical way of handling this, even by the JDK itself and we don’t have to care about throwing Exceptions.

Candidates: Please wait until we’ve closed the discussion happening here before starting to replace the 13 occurences.

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Comments: 18 (15 by maintainers)

Commits related to this issue

Most upvoted comments

Kotlin is a non-starter. This is a Java engine. Fork it and make kmonkeyengine if you like but I’m 100% sure we are not switching to kotlin.

As to the other, I will repeat and summarize: Step 1: search replace NullPointerException(" with IllegalArgumentException(" (note the quotation mark) as to me that one is the ‘no brainer’. Step 2: search for “new NullPointerException” if found, EITHER convert to IllegalArgumentException with an appropriate message or convert assert != null. A judgment call based on context.

Done.

If Java adds a standard “nullable” annotation of some kind then we can worry about that. I don’t think we should suck in a separate library just for that. (And anyway, that’s a separate issue from all of the manual NPEs we throw.)

requiresNonNull() is no better than manually throwing NPEs.

The hill I will die on: “NullPointerException should always happen on the line that the null pointer was dereferenced.” ie: always thrown by java as the result of a bad dereference. User code that throws it (and I’m including the abominations that are requireNonNull()) are diluting the utility of the “single easiest exception to diagnose”.

Just to play devil’s advocate, but if null checking checking is a big enough performance concern then a switch to something like Kotlin which does null checking at compile time could be worth entertaining.

I know that this move comes with huge implications least of which would be the massive effort required. However, Kotlin is evolving very rapidly and I feel that it could provide benefits such as common codebase for all targets as well as compile time null safety.

Then again I’m just getting started on using jMonkey and contributing so I might change my tune later lol

I would HIGHLY recommend against this. As someone who develops enterprise software, manages a team of developers, and having worked on kotlin enterprise applications; kotlin is good for small applications that are one or two developer projects, but it does not scale to large applications that require a large team to maintain. Maintainability in kotlin is a nighmare. Kotlin is also a proprietary DSL owned by JetBrains, this would limit jme’s development to their environment and when they decide to stop supporting it we would be SOL. As far as performance, I have seen major performance impacts in kotlin applications that would not have existed in java applications do to the extra support byte code that kotlin generates during compile time to enforce things like null checks. Kotlin has some very fancy features, but most of those are counter productive to stable and maintainable code development.

I’m not trying to be mean or start any fires, I just want to share my experience as I have already suffered this scenario before on large projects. Null protection always sounds great at a language level until you have to live with it.

It seems the general assumption is that kotlin is an improvement over java. I am against this assumption, kotlin is a language that is made for people that dislike java for its design (sometimes due to misconceptions), and i believe using most of kotlin’s features in the core would impact performances negatively.

Again, like I said, I was playing devil’s advocate. I’m definitely a fan of Kotlin but I recognize the investment the team has made in their current stack and others hesitations.

I also work on large enterprise software and supporting code bases that have been around for much longer than JME so I definitely get it. Where I’m at we’re primarily a Java shop with the core of our product written in decades old and highly optimized C++. Where I’m lacking is having had worked with Kotlin in a large project with a team. @tlf30 I’m interested to know in more detail your takes on Kotlin from your experiences. Doesn’t have to be here. You can shoot me an email.

Anyways, back on track. Are there not any not null annotations in the stdlib? I could’ve swore I saw some on the classpath. Hang on…

10 minutes later

Okay so the only thing I’ve found that’s even close to being in the stdlib is javax.annotations.NonNull but that looks like it’s a separate maven dep. Darn.

Therefore, I agree with @pspeed42 that NPEs should always throw from the line they happen on. It also lines up with my personal view of “fail fast and fail hard”.

I don’t believe in “customized null pointer exceptions” when we really mean “this argument is bad”.

The line of the NPE should always be an actual NPE. To me there are no exceptions. (hehe)

I also thought for long that IllegalArgumentException should be used. This seems to be a topic that divides the community roughly in half. Even the winning answer in Stackoverflow debate recommends IllegalArgumentException. But the runner up has really valid points and given Objects.requireNonNull javadoc… I would go against Stackoverflow’s chosen answer… I mean it is a peasant revolt I have done only now twice…

So if my vote counts, I vote to follow @MeFisto94 's suggestion order 😃