No return null (get territory)#1.1 #13614
Conversation
*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
fix pmdMain RulesAttachment.java
fix NotNull to Nonnull
| private Optional<Territory> getTerritory(@Nullable String territoryName) { | ||
| return Optional.ofNullable(getData().getMap().getTerritoryOrNull(territoryName)); | ||
| } | ||
|
|
||
| @Nonnull | ||
| protected Territory getTerritoryOrThrowGameParseException(String territoryName) | ||
| throws GameParseException { | ||
| return getTerritory(territoryName) | ||
| .orElseThrow( | ||
| () -> new GameParseException("No territory named: " + territoryName + thisErrorMsg())); | ||
| } | ||
|
|
||
| @Nonnull | ||
| protected Territory getTerritoryOrThrowIllegalStateException(@Nullable String territoryName) | ||
| throws IllegalStateException { | ||
| return getTerritory(territoryName) | ||
| .orElseThrow( | ||
| () -> | ||
| new IllegalStateException("No territory named: " + territoryName + thisErrorMsg())); |
There was a problem hiding this comment.
Same issue here as the other PR that is already reviewed. I don't know if it was cleanly stated... I sent you a branch with suggestions on how to fix this.
Okay.. so these two methods getTerritoryOrThrowGameParseException and getTerritoryOrThrowIllegalStateException are really showing inappropriate error handling. The two methods are there for the sake of the caller being able to call the right one for the right context to the error handling. The caller knows the context of the error, the caller should be the one attaching the right exception type.
A very old school Java approach would be to have these methods be unified and throw a TerritoryNotFoundException. The caller would in turn catch that and throws the right one. If the caller can instead grab all needed data for an error message, then it's considerably cleaner to return an empty optional and have the caller do the error handling.
Having checked exceptions is very weird and makes coding difficult.. I recommend going with the route in the branch I sent your way..
2 participants
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.
Replacement of PR #13584.