Collection of minor improvements (#259)

* Use random.choice in RandomPlayer to pick random action.

* Use dataclass with LandTile

* Use dataclass with Port

* Use random.choice in WeightedRandomPlayer to pick random action.

* Simplify class member initialization in from_tiles method.

* Use .values() instead of .items() when iterating just over values

* Simplify maintain_longest_road logic.

* Simplify maintain_largest_army logic.

* Prefer .values() over .items() in robber_possibilities if iterating over just values.

catanatron_core/catanatron/models/actions.py

@@ -209,7 +209,7 @@ def robber_possibilities(state, color) -> List[Action]:
         # each tile can yield a (move-but-cant-steal) action or
         #   several (move-and-steal-from-x) actions.
         to_steal_from = set()  # set of player_indexs
-        for _, node_id in tile.nodes.items():
+        for node_id in tile.nodes.values():
             building = state.board.buildings.get(node_id, None)
             if building is not None:
                 candidate_color = building[0]

catanatron_core/catanatron/models/map.py

@@ -45,41 +45,30 @@ class EdgeRef(Enum):
 Coordinate = Tuple[int, int, int]
 
 
+@dataclass
 class LandTile:
-    def __init__(
-        self,
-        tile_id: int,
-        resource: Union[FastResource, None],
-        number: Union[int, None],
-        nodes: Dict[NodeRef, NodeId],
-        edges: Dict[EdgeRef, EdgeId],
-    ):
-        self.id = tile_id
-
-        self.resource = resource  # None means desert tile
-        self.number = number  # None if desert
+    id: int
+    resource: Union[FastResource, None]  # None means desert tile
+    number: Union[int, None]  # None if desert
+    nodes: Dict[NodeRef, NodeId]  # node_ref => node_id
+    edges: Dict[EdgeRef, EdgeId]  # edge_ref => edge
 
-        self.nodes = nodes  # node_ref => node_id
-        self.edges = edges  # edge_ref => edge
-
-    def __repr__(self):
-        if self.resource is None:
-            return "Tile:Desert"
-        return f"Tile:{self.number}{self.resource}"
+    # The id is unique among the tiles, so we can use it as the hash.
+    def __hash__(self):
+        return self.id
 
 
+@dataclass
 class Port:
-    def __init__(
-        self, port_id, resource: Union[FastResource, None], direction, nodes, edges
-    ):
-        self.id = port_id
-        self.resource = resource  # None means its a 3:1 port.
-        self.direction = direction
-        self.nodes = nodes
-        self.edges = edges
+    id: int
+    resource: Union[FastResource, None]  # None means desert tile
+    direction: Direction
+    nodes: Dict[NodeRef, NodeId]  # node_ref => node_id
+    edges: Dict[EdgeRef, EdgeId]  # edge_ref => edge
 
-    def __repr__(self):
-        return "Port:" + str(self.resource)
+    # The id is unique among the tiles, so we can use it as the hash.
+    def __hash__(self):
+        return self.id
 
 
 @dataclass(frozen=True)
@@ -264,7 +253,7 @@ def from_tiles(tiles: Dict[Coordinate, Tile]):
         self.port_nodes = init_port_nodes_cache(self.tiles)
 
         land_nodes_list = map(lambda t: set(t.nodes.values()), self.land_tiles.values())
-        self.land_nodes = frozenset(set.union(*land_nodes_list))
+        self.land_nodes = frozenset().union(*land_nodes_list)
 
         # TODO: Rename to self.node_to_tiles
         self.adjacent_tiles = init_adjacent_tiles(self.land_tiles)

catanatron_core/catanatron/models/player.py

@@ -76,5 +76,4 @@ class RandomPlayer(Player):
     """Random AI player that selects an action randomly from the list of playable_actions"""
 
     def decide(self, game, playable_actions):
-        index = random.randrange(0, len(playable_actions))
-        return playable_actions[index]
+        return random.choice(playable_actions)

catanatron_core/catanatron/players/weighted_random.py

@@ -23,5 +23,4 @@ def decide(self, game, playable_actions):
             weight = WEIGHTS_BY_ACTION_TYPE.get(action.action_type, 1)
             bloated_actions.extend([action] * weight)
 
-        index = random.randrange(0, len(bloated_actions))
-        return bloated_actions[index]
+        return random.choice(bloated_actions)

catanatron_core/catanatron/state.py

@@ -261,7 +261,7 @@ def yield_resources(board: Board, resource_freqdeck, number):
         if tile.number != number or board.robber_coordinate == coordinate:
             continue  # doesn't yield
 
-        for _, node_id in tile.nodes.items():
+        for node_id in tile.nodes.values():
             building = board.buildings.get(node_id, None)
             assert tile.resource is not None
             if building is None:

catanatron_core/catanatron/state_functions.py

@@ -25,41 +25,47 @@ def maintain_longest_road(state, previous_road_color, road_color, road_lengths):
     for color, length in road_lengths.items():
         key = player_key(state, color)
         state.player_state[f"{key}_LONGEST_ROAD_LENGTH"] = length
-    if road_color is None:
-        return  # do nothing
 
-    if previous_road_color != road_color:
-        winner_key = player_key(state, road_color)
-        state.player_state[f"{winner_key}_HAS_ROAD"] = True
-        state.player_state[f"{winner_key}_VICTORY_POINTS"] += 2
-        state.player_state[f"{winner_key}_ACTUAL_VICTORY_POINTS"] += 2
-        if previous_road_color is not None:
-            loser_key = player_key(state, previous_road_color)
-            state.player_state[f"{loser_key}_HAS_ROAD"] = False
-            state.player_state[f"{loser_key}_VICTORY_POINTS"] -= 2
-            state.player_state[f"{loser_key}_ACTUAL_VICTORY_POINTS"] -= 2
+    # If road_color is not set or is the same as before, do nothing.
+    if road_color is None or (previous_road_color == road_color):
+        return
+
+    # Set new longest road player and unset previous if any.
+    winner_key = player_key(state, road_color)
+    state.player_state[f"{winner_key}_HAS_ROAD"] = True
+    state.player_state[f"{winner_key}_VICTORY_POINTS"] += 2
+    state.player_state[f"{winner_key}_ACTUAL_VICTORY_POINTS"] += 2
+    if previous_road_color is not None:
+        loser_key = player_key(state, previous_road_color)
+        state.player_state[f"{loser_key}_HAS_ROAD"] = False
+        state.player_state[f"{loser_key}_VICTORY_POINTS"] -= 2
+        state.player_state[f"{loser_key}_ACTUAL_VICTORY_POINTS"] -= 2
 
 
 def maintain_largest_army(state, color, previous_army_color, previous_army_size):
     candidate_size = get_played_dev_cards(state, color, "KNIGHT")
-    if candidate_size >= 3:
-        if previous_army_color is None:
-            winner_key = player_key(state, color)
-            state.player_state[f"{winner_key}_HAS_ARMY"] = True
-            state.player_state[f"{winner_key}_VICTORY_POINTS"] += 2
-            state.player_state[f"{winner_key}_ACTUAL_VICTORY_POINTS"] += 2
-        elif previous_army_size < candidate_size and previous_army_color != color:
-            # switch, remove previous points and award to new king
-            winner_key = player_key(state, color)
-            state.player_state[f"{winner_key}_HAS_ARMY"] = True
-            state.player_state[f"{winner_key}_VICTORY_POINTS"] += 2
-            state.player_state[f"{winner_key}_ACTUAL_VICTORY_POINTS"] += 2
-            if previous_army_color is not None:
-                loser_key = player_key(state, previous_army_color)
-                state.player_state[f"{loser_key}_HAS_ARMY"] = False
-                state.player_state[f"{loser_key}_VICTORY_POINTS"] -= 2
-                state.player_state[f"{loser_key}_ACTUAL_VICTORY_POINTS"] -= 2
-        # else: someone else has army and we dont compete
+
+    # Skip if army is too small to be considered.
+    if candidate_size < 3:
+        return
+
+    if previous_army_color is None:
+        winner_key = player_key(state, color)
+        state.player_state[f"{winner_key}_HAS_ARMY"] = True
+        state.player_state[f"{winner_key}_VICTORY_POINTS"] += 2
+        state.player_state[f"{winner_key}_ACTUAL_VICTORY_POINTS"] += 2
+    elif previous_army_size < candidate_size and previous_army_color != color:
+        # switch, remove previous points and award to new king
+        winner_key = player_key(state, color)
+        state.player_state[f"{winner_key}_HAS_ARMY"] = True
+        state.player_state[f"{winner_key}_VICTORY_POINTS"] += 2
+        state.player_state[f"{winner_key}_ACTUAL_VICTORY_POINTS"] += 2
+
+        loser_key = player_key(state, previous_army_color)
+        state.player_state[f"{loser_key}_HAS_ARMY"] = False
+        state.player_state[f"{loser_key}_VICTORY_POINTS"] -= 2
+        state.player_state[f"{loser_key}_ACTUAL_VICTORY_POINTS"] -= 2
+    # else: someone else has army and we dont compete
 
 
 # ===== State Getters