No return null (get territory)

[frigoref]

noReturnNull (getTerritory) #5

GameMap.java

• method getTerritoryOrNull: Replaced by getTerritory
• new method getTerritory: Returning Optional and replacing each place formerly calling getTerritoryOrNull

ProSimulateTurnUtils.java

• methods transferMoveMap/transferUnit/transferLoadedTransport: Use getTerritoryOrThrow instead of getTerritoryOrNull

noReturnNull (getTerritory) #4

AbstractPlaceDelegate.java
BidPlaceDelegate.java
IAbstractPlaceDelegate.java

• methods placeUnits/isValidPlacement/canProduce/getAllProducers/checkProduction/getBestProducerComparator: Annotate territory parameter NotNull

ProPlaceTerritory.java
ProPurchaseTerritory.java

• Constructor/attribute territory: Annotate territory NotNull

ProPurchaseAi.java

• method place: Rename data to proGameData and use getTerritoryOrThrow as null is not handled
• method doPlace: Annotate territory parameter NotNull

ProSimulateTurnUtils.java

• method simulateBattles: remove re-read territory after owner change (new data already updated)

noReturnNull (getTerritory) #3
triplea\Constants.java

• Importing further file name/path constants

Others:

• Adjust to extraction of file constants

noReturnNull (getTerritory) #2
DecorationPlacer.java

• new methods loadPolygonsFile, loadCentersFile, selectAndLoadPolygonsFile, selectAndLoadCentersFile: Helper methods to load respective files

PlacementPicker.java

• Constructor: replace load code for polygons with new method loadPolygonsFile

PolygonGrabber.java

• Constructor: replace load code for centers with new method loadCentersFile

MapData.java

• Extraction of file name constants into Constants.java

triplea\Constants.java

• Importing file name constants

Others:

• Adjust to renaming/moving constants

noReturnNull (getTerritory)
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

• Replaces null return on getTerritory
• Introduces new method getTerritoryOrThrow for cases where the territory is expected to be found
• Extracts file name constants into triplea\Constants.java
• DecorationPlacer.java
• new methods loadPolygonsFile, loadCentersFile, selectAndLoadPolygonsFile and selectAndLoadCentersFile in DecorationPlacer.java to encapsulate helper logic to load respective files

[DanVanAtta]

@frigoref I think you might have went a bit overboard loading up this PR. It's going to be hard to get this tested adequately, reviewed & merged. I recommend discussing some of the types of refactorings offline and then breaking this PR up into multiple.

[DanVanAtta]

There were prior efforts to consolidate the "not null" annotation used. There's a javax variant that requires no extra libraries. Please use: import javax.annotation.Nonnull;

[frigoref]

Adjusted in new partial PR #13584

[DanVanAtta]

You can remove @NotNull on methods. By default it's assumed methods will not return null and therefore you don't need @NotNull. The codebase violates that assumption all over the place, but at the same time the @NotNull does not actually do much whether the assumption holds or is violated.

[frigoref]

Adjusted in new partial PR #13584. It was used to differentiate it from method getTerritoryOrNull.

[DanVanAtta]

@Nullable parameter is a bit sus.
The "rule" is "never pass null, never return null". So, we should avoid calling getTerritory if we know territoryName is null. By that token, we really should not call getTerritoryOrThrowIllegalStateException if territoryName is null. Can we move any null checking to the caller?

[DanVanAtta]

Second comment, adding methods to throw specific exception types is dirty. This looks to be a case where Optional should be returned and then the caller deals with that. Or, if there is context that the caller can't get, then throw a specific exception type and have the caller catch that and then convert the exception type to what they need.

The issue is we have low level code that is trying to do everything, it's trying to do the correct error handling for the code that is at higher layers. This code does not have enough information to know what the correct exception type would be, which means it's the caller that will need to do some exception handling.

[frigoref]

This is a helper method to make more clear by name what it does. The usage has not been affected and whether it is used with null or not can be addressed afterwards, no?
The current change is about to get rid of returning null on getTerritory

[DanVanAtta]

I pushed a branch to you with this pattern all fixed up.. Please check that for a pretty working example of one way to fix this.

A classic "java" fix would be to have this lower layer throw a single exception type. Another approach would be to use the one method that returns an Optional and to have the caller do something like: getTerritory(name).orElseThrow(() -> new IllegalArgumentException...

The proposed update is very similar to the following (notice how the parameter signature allows the caller of this method to tell this method what to throw):

protected Territory getTerritoryOrThrowIllegalStateException(@Nullable String territoryName, Function<String, RuntimeException> exceptionToThrowFactory)
 
  final Territory t = territoryLookup.get(territoryName);
  if (t == null) {
       throw exceptionToThrowFactor.apply(
            MessageFormat.format("Territory with name {0} could not be found", territoryName));
      }
  return t;
}

[DanVanAtta]

Slightly simpler:

return getTerritory(territoryName)
                 .orElseThrow(() -> new IllegalArgumentException(
                            MessageFormat.format("Territory with name {0} could not be found", territoryName));

Side note, we use String.format more frequently. I somewhat think we should prefer consistency and avoid using MessageFormat unless we specifically need something in MessageFormat that String.format does not provide.

edit was missing the () -> new IllegalArgumentException( in the example, (second line)

[frigoref]

k, will provide another PR at some point to replace all the Message Format parts I have done so far (that are quite a few).

[DanVanAtta]

k, will provide another PR at some point to replace all the Message Format parts I have done so far (that are quite a few).

The consistency between MessageFormat and String.format is not a huge deal IMO. Assuming you haven't made changes yet. If you haven't already changed things, it's fine to leave things as they are IMO.

[DanVanAtta]

Suggested change

	final Territory t = territoryLookup.get(territoryName);
	if (t == null) {
	throw new IllegalArgumentException(
	MessageFormat.format("Territory with name {0} could not be found", territoryName));
	}
	return t;
	return getTerritory(territoryName)
	.orElseThrow(() -> new IllegalArgumentException(
	MessageFormat.format("Territory with name {0} could not be found", territoryName));

This is the main suggestion. The above (needs formatting), but will help re-use existing code.
Though, this point could be moot. If getTerritory is public, then a caller can do this type of code with just a couple lines, and attach the right error message & right error type as well.

[DanVanAtta]

Let's stop decorating method parameters so much. I don't believe the final and @NotNull help.
The nullity annotations are largely for an IDE to help us. IDE's are smart enough to infer a @NotNull constraint by how variables are used. So, you're getting the NotNull annotation for free without explicitly declaring it. Meanwhile there is a drawback where @NotNull final is adding noise.

[frigoref]

I do not see that the IDE is making this automatically and as apparently the Territory can be passed sometimes as null I think it is better at the moment to state clearly when this is not the case.
We can remove it later, but at the moment I always have to check whether it might be having value null or not.
Concerning annotation final I would prefer the IDE to work the other way around with method parameters, i.e., only having to declare when they are not final, but sadly it is not that way and therefore, I would vote to keep it for clarity purposes.

[DanVanAtta]

I had the same discussion with other devs within TripleA regarding adding @NotNull, the conclusion was to not add it.

That you don't see the annotation being added, it is added when the IDE is doing nullity analysis. It doesn't physically add it in anything you would see.

I think it is better at the moment to state clearly when this is not the case.

And what's the chance it gets out of date? It's still not clear.

Second, we still pay costs for having the noise. A param list that could fit on one line is instead 50% keywords. Furhter, why not add @NotNull to cond? In this example, we have raised additional questions there. Given there can't be certainty that even @NotNull is correct, we've only added questions.

"We can remove it later, but at the moment I always have to check whether it might be having value null or no"

If adding the annotation is a waste of time to begin with, it's not compelling to suggest that we can simply remove it later.

[DanVanAtta]

I suspect things like blockade.txt is used in multiple places and is hardcoded. By that token, extracting to a constant is a good thing. Constants is not a great place for it. The tools package is almost its own application. We should avoid creating any dependency between the tools and the rest of the source code as much as we can.

[frigoref]

When extracting I tried to extract the same text from all files in the project and replace it with the constant.
I also thought about package dependency and there might be a need of a "common" package that carries for example shared constants and the multiple packages that might use these constants. What do you think about that?

[DanVanAtta]

A single constants class is likely to create a lot of coupling.

More ideally we could modularize and completely hide these constants. Specifically, today triplea-game/game-app/map-data is the only thing that does not depend on triplea-game/game-app/game-core. If, for example, we moved this tools utility into its own module that did not depend on game-core, we could keep more constants scoped to just within the module. This ideal solution is more difficult, but for sure something that is desired. I really believe TripleA code is best cleaned up by creating a number of "low layer" modules that site below game-core.

Short of that goal though, instead of putting constants into a single top level package, we could also create multiple constant files. If we try to keep the constants as "close" as possible to the files that use it, that would be a relatively good step in the right direction. Though.. in some cases, some files should be the proper owner of a constant and everything else should reference it. If we collapse all common usages together, we might lose that ownership criteria.

To explain what I mean by "proper owner" - For example, maybe there is a value that should be "owned" by GameData. In this case the value should not be in Constants.java. When used, it should not be Constants.VALUE, but it properly should be GameData.VALUE. Hence, whether something is even extracted to a constant depends on usage and the concept of "ownership", how we want to achieve data-hiding (AKA encapsulation).

[DanVanAtta]

This comment could use a bit more context. What changed the territory object and how has it changed??

[frigoref]

If you just read this line, but if you also read the next one I think it becomes clear, don't you?

ProLogger.debug("after changes owner=" + t.getOwner() + ", units=" + t.getUnits());

[DanVanAtta]

The comment is good for a commit, it makes sense when looking at the removal code next to the code added.

I can see that this line: final Territory updatedTerritory = data.getMap().getTerritory(t.getName()); is not needed thanks to the comment. A future maintainer of the code is not going to see this diff, the comment won't make sense to them. That's why I said the comment could use more context. The fact we only log a few things kinda indicates that something has changed. It's therefore confusing why there is a call-out that the territory object has changed, it seems obvious already.

In honesty, the comment ought to be part of a commit message and this update its own PR.

[frigoref]

Not sure exactly what is this conversion is aiming at anymore. We started with

This comment could use a bit more context. What changed the territory object and how has it changed??

To me my response answers this questions and I do not see what you still see as open. Wouldn't it be better you provide a comment that you see fit to speed up things?

[DanVanAtta]

This is what I see as a reviewer:

I agree that it all makes sense. I can see variable updatedTerritory, I can see the variable is removed, I can see the comment explaining why the variable is removed. It makes sense.

---

OTOH, after we merge this, this block will become:

Now, AFAIK, the comment no longer makes sense. The comment refers to updatedTerritory, but that was deleted. So, the comment is referring to deleted code... Otherwise, there's a comment on a ProLogger.debug statement, which is kind of an "interesting" in its own right..

I still recommend you just strike this comment (or if there is something really subtle, to call that out explicitly in the code comment)

[DanVanAtta]

Suggested change

// note that the Territory object has already changed