Update Default Player Type to String For Flexibility #2048
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2048 +/- ##
============================================
+ Coverage 18.71% 18.71% +<.01%
Complexity 5405 5405
============================================
Files 777 777
Lines 77702 77709 +7
Branches 12914 12917 +3
============================================
+ Hits 14539 14541 +2
- Misses 61104 61111 +7
+ Partials 2059 2057 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Code looks good, @ron-murhammer. One potential (but unlikely) NPE identified below.
| @@ -73,26 +77,26 @@ public void actionPerformed(final ActionEvent e) { | |||
| } | |||
| if (!(previousSelection.equals("no_one")) && Arrays.asList(types).contains(previousSelection)) { | |||
| playerTypes.setSelectedItem(previousSelection); | |||
| } else if (player.isAiDefault()) { | |||
| } else if (player.getDefaultType().equals(PLAYER_TYPE_AI)) { | |||
There was a problem hiding this comment.
You might want to swap the order of the comparison here because PlayerID#getDefaultType() can return null. It looks like that should never happen for PlayerID instances created via GameParser, but there is a public constructor which does set the corresponding field to null (although it appears it's only used by the tests at this time).
| // the 4th in the list should be Pro AI (Hard AI) | ||
| playerTypes.setSelectedItem(types[Math.max(0, Math.min(types.length - 1, 3))]); | ||
| } else if (player.getDefaultType().equals(PLAYER_TYPE_DOES_NOTHING)) { |
| @@ -23,27 +23,31 @@ | |||
|
|
|||
| class PlayerSelectorRow { | |||
|
|
|||
| private static final String PLAYER_TYPE_AI = "AI"; | |||
| private static final String PLAYER_TYPE_DOES_NOTHING = "DoesNothing"; | |||
There was a problem hiding this comment.
Just thinking out loud... Is there a better place for these constants (including one for "Human")? Since you're defining the legal values in the DTD, that would suggest they're API and should be defined in the engine domain model. I'm guessing this feature is still evolving, and you'll probably move them when everything becomes better focused. 😄 (An enum may even be in its future given that the DTD limits it to a finite set of values.)
There was a problem hiding this comment.
Yeah, I thought the same thing but decided to leave them just in this class for now.
|
Merging this now after addressing the review comments. |
Further addresses: https://forums.triplea-game.org/topic/132/handling-of-ai-players-not-meant-to-be-played-github-request
Functional Changes:
Examples:
<player name="Germans" optional="false" defaultType="AI" /><player name="Italians" optional="false" defaultType="DoesNothing" />