No return null (get territory)#1

[frigoref]

Test.java

• usage of method getTerritoryOrThrow instead of getTerritory

DefaultAttachment.java

• method getTerritoryOrThrow: replaces with getTerritoryOrThrowGameParseException
• new methods getTerritoryOrThrowGameParseException/getTerritoryOrThrowIllegalStateException: Throwing specific exception when no territory was found
• new method getTerritory: private method returning Optional to avoid returning null

GameMap.java

• method getTerritory: Renamed to getTerritoryOrNull
• method getTerritoryOrThrow: Documentation added
• methods getDistance: Parameter annotation NotNull added

GameSelectorModel.java

• method parseAndValidate: Return Optional instead of null
• method setGameData: Parameter annotation Nullable added

GameTestUtils.java

• method getTerritory: Replaced with getTerritoryOrThrow

UnitTypeList.java

• method getUnitTypes: Documentation updated

Notes to Reviewer

Follow-up PRs shall get to a state that getTerritory is no longer returning null.

[DanVanAtta]

@frigoref I pushed a branch with some changes I would recommend, to pulls those in (if you choose), you can cherry-pick the last few commits from: https://github.com/triplea-game/triplea/tree/updates

[DanVanAtta]

The naming of getTerritoryOrNull is a bit odd IMO.

To be frank, the renaming is not quite helping. It is already clear we can have a nullable return value based on the annotation.

The problem is the nullable return value.

What we really want is the return value to be not-nullable. The cheapest way to do that is to return Optional<Territory>. There is some AI code that uses this method and would NPE, updating the return type forces the caller to deal with the potential nullable return value. That is what we really want. We don't want to just make it clear to the caller that there is a nullable return value, but instead compile-time enforce that the caller deals with the nullable.

In other words, we want to do compile time enforcement that callers of nullable APIs handle the potential null value, rather than just communicating that through naming or annotations.

[frigoref]

The naming is temporary (until the other parts of the big PR #13579 are also in) and at the moment for the purpose of making it easy to spot.

[DanVanAtta]

Any reason to not use @Nonnull available from line 24?

There was once a PR some time ago that unified all the nullability annotations to javax some time ago. AFAIK that is the one that is the most consistent to existing project usages.

[frigoref]

I did the same replacement again as with the other PR and provided new commit noReturnNull (getTerritory) #1.1.

[DanVanAtta]

Annotating parameters with @NotNull does not necessarily help a lot.

Any new code should never pass a null value as a parameter.

NPEs are a billion dollar design problem. Billions have been spent on the NPE problem. AFAIK modern java does not have NPE problems. The rules are:

• never pass null to a method
• never return null

So, what does this annotation do? For new code, nobody should be passing null in the first place. For existing code - this annotation is already automatically inferred by the IDE null linter, so we get no better detection of NPEs.

At least that is the rational for why I say we never need @NotNull on method parameters. It's not really helpful for new or existing code. Except... when we are dealing with frameworks, like lombok, that actually do make use of the annotations, and it has a material impact on the bytecode that is produced and executed at runtime. In particular the jetbrains @NotNull is discarded before runtime, so it's purely there for IDE & and communication to a developer.

Instead the interesting thing is when a parameter is marked as @Nullable. That is new information to the IDE and linters and it will highlight potential NPEs. Grant it, yes - a nullable parameter is bad design, but annotating it as such does have a material impact on new linter warnings.

[frigoref]

The reason I introduced this here is that it was hard to understand from the code where a Territory parameter could be null and where not. The assumption it is always Nonnull by default did not hold up several time.
But maybe a better way to handle this is use Preconditions.checkNotNull.
If you agree, I would rework those cases.
Note, that the current cases are reworked to Nonnull annotation already with commit noReturnNull (getTerritory) #1.1.

[DanVanAtta]

The assumption it is always Nonnull by default did not hold up several time.

Yeah, it won't. Most of the code was written before better nullity conventions were established and null is used like crazy. Even annotating a method with @NotNull is going to really give you that guarantee either.

But maybe a better way to handle this is use Preconditions.checkNotNull

That is not terrible. The Precondition check stays around in the compiled code. Though we need to be sure we are not materially changing any of the existing behaviors. Unless @Notnull is interfacing with lombok, or actively giving you a linting validation, it's not a terribly helpful annotation.

Preconditions.checkNotNull.
If you agree, I would rework those cases.

Why is the caller not already checking that the territory value is not null to begin with? In part a key is to define a logical layer where there is null checking. If we expect callers to pass 'null', then it's not an error. If it is an error to pass null, then the error has already happened at the point of the caller. The problem is upstream, we should enforce the non-null upstream..

Ideally when a system is built there is defined some layer, a boundary, where things are checked to be valid and then behind that boundary you know that things are reasonably good looking. When a system gets complicated, it can be hard to understand where that layer should be (or used to be), and just start throwing defensive checks in the code. What's worse, early java programmers often had no such concept, and used null as a special data type and the results are gnarly layers of if statements and complexity.

I think overall you can do a little less work, you're kinda going out of your way to make updates in nearby code, which is blowing up the scope of your PR.

[DanVanAtta]

are you sure this shouldn't be getTerritoryOrNull(name) (or better yet: getTerritory(name).orElse(null))

I'm having trouble telling if this would be a material change. The previous code was able to handle null values coming through here. Whether that resulted in an error downstream or not, I don't know. If the downstream code did not throw any type of errors, then throwing an error now would be a change in behavior. This is actually quite scary code to change because it might be executed in a very niche manner. I think you would be best off to do some local testing and hardcode a null return value here and then see what blows up. If something does, then you'll have a potential code path that does throw an error, and therefore throwing an error upsteam is a good thing.

[frigoref]

I thought of a fail-early approach as I assumed the streaming process not getting a territory by its name is a strong indication that there is something wrong.
This follows the idea of case UNITTYPE.
In general, I would not assume that the streaming process will call this method with a name to which it cannot find anything (Territory or UnitType).

[DanVanAtta]

In general, I would not assume that the streaming process will call this method with a name to which it cannot find anything (Territory or UnitType).

Yeah, that would be a sane assumption. Who knows what the reality is. We have to check it.

[DanVanAtta]

I'm at about 30% reviewed. I recommend to break up these updates even further.

[frigoref]

I'm at about 30% reviewed. I recommend to break up these updates even further.

I understand it is still a big commit, but I fear to mess up more and we will need more time if we do not pull through with this one. At the end, the remaining commits are smaller.

[frigoref]

I must have messed it up nicely with rebase and so on, so I opened a new clean PR #13614 that contains the original 3 commits and the rework according to your comments.
Sorry, for the mess up.

[DanVanAtta]

I must have messed it up nicely with rebase and so on, so I opened a new clean PR #13614 that contains the original 3 commits and the rework according to your comments.
Sorry, for the mess up.

No worries. Just please close down what is out-of-date, and assign (and/or "re-request") PRs back to me for anything you want me to check out again. Thank you for the work & care you show for this project @frigoref . It is appreciated.