refactor: Clean Code refactoring — DRY entity hierarchy, decompose WorldScreen, remove duplication, DI audit#351
Conversation
…oggles and screen switches Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/5290e10f-d18d-4083-8927-097575ce6e81 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
- Delegate save/load to WorldScreenPersistence helper class - Delegate canBePickedUp to pickupableEntities module - Extract methods: _drawDayNightOverlay, _drawHotbar, _drawDebugInfo from draw() - Extract methods: _processEvents, _updateLivingEntities, _updateGameState from run() - Extract methods: _loadOrGenerateRoom, _calculateTargetLocationForRoomTransition from changeRooms() - Extract _plantWheatSeed from executePlaceAction() - Extract _handleMovementKey, _handleHotbarKey from handleKeyDownEvent() - Extract _handleHotbarClick, _handleWorldClick from handleMouseDownEvent() - Consolidate cooldown methods into _checkCooldown(tickToCheck, speed) - Simplify handleMouseWheelEvent with modulo arithmetic - Remove 14 unused entity imports and jsonschema import - File reduced from 2122 to 1963 lines (-159 lines) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
…ing to except blocks, update CHANGELOG Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/5290e10f-d18d-4083-8927-097575ce6e81 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
…handleUtilityKey to remove elif pass Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/5290e10f-d18d-4083-8927-097575ce6e81 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors core gameplay screens and entity classes to remove duplication (DRY), decompose oversized methods in WorldScreen, and centralize persistence/pickup logic while also improving logging around previously silent failures.
Changes:
- Centralized
solid/isSolid()inDrawableEntityand removed redundant implementations across many entity subclasses. - Decomposed
WorldScreeninto focused helpers and extractedWorldScreenPersistence+pickupableEntities. - Refactored
ConfigScreen/OptionsScreento be data-driven, and replaced silentOSErrorhandling with warnings.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/world/room.py | Removes redundant comments while preserving behavior. |
| src/screen/worldScreenPersistence.py | New helper class encapsulating WorldScreen save/load logic. |
| src/screen/worldScreen.py | Decomposes large methods; delegates persistence/pickup logic; adds helpers for overlay/hotbar/debug. |
| src/screen/pickupableEntities.py | Centralizes pickupable-type checks into a shared tuple + helper. |
| src/screen/optionsScreen.py | DRYs screen switching and button rendering. |
| src/screen/inventoryScreen.py | Removes redundant comments; no functional change intended. |
| src/screen/configScreen.py | Replaces many toggles with attribute-driven toggle list. |
| src/screen/codexScreen.py | Formatting-only change to structured error log. |
| src/gameLogging/logger.py | Formatting-only changes to logger configuration/constants. |
| src/entity/drawableEntity.py | Adds solid constructor arg + base isSolid() implementation. |
| src/entity/food.py | Renames ctor parameter to imagePath (clarity) while preserving behavior. |
| src/entity/apple.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/banana.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/bearMeat.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/bed.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/campfire.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/chickenMeat.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/coalOre.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/excrement.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/fence.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/grass.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/ironOre.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/jungleWood.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/leaves.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/matureCrop.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/oakWood.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/stone.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/stoneBed.py | Passes solid=True to base instead of local boilerplate. |
| src/entity/stoneFloor.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/torch.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/wheat.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/wheatSeed.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/woodFloor.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/youngCrop.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/living/bear.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/entity/living/chicken.py | Removes redundant solid/isSolid boilerplate (inherits default non-solid). |
| src/config/keyBindings.py | Adds warning log on save failure (replaces silent pass). |
| src/config/config.py | Adds warning log on window size save failure (replaces silent pass). |
| CHANGELOG.md | Documents the refactor and adds learning-log entries. |
| def _loadOrGenerateRoom(self, x, y): | ||
| room = self.map.getRoom(x, y) | ||
| if room == -1: | ||
| # attempt to load room if file exists, otherwise generate new room | ||
| nextRoomPath = ( | ||
| self.config.pathToSaveDirectory | ||
| + "/rooms/room_" | ||
| + str(x) | ||
| + "_" | ||
| + str(y) | ||
| + ".json" | ||
| if room != -1: | ||
| return room | ||
| nextRoomPath = self.persistence.buildRoomPath(x, y) | ||
| if os.path.exists(nextRoomPath): | ||
| roomJsonReaderWriter = RoomJsonReaderWriter( | ||
| self.config.gridSize, self.graphik, self.tickCounter, self.config | ||
| ) | ||
| if os.path.exists(nextRoomPath): | ||
| roomJsonReaderWriter = RoomJsonReaderWriter( | ||
| self.config.gridSize, self.graphik, self.tickCounter, self.config | ||
| ) | ||
| room = roomJsonReaderWriter.loadRoom(nextRoomPath) | ||
| self.map.addRoom(room) | ||
| self.currentRoom = room | ||
| self.status.set("Area loaded") | ||
| else: | ||
| x, y = self.getCoordinatesForNewRoomBasedOnPlayerLocationAndDirection() | ||
| self.currentRoom = self.map.generateNewRoom(x, y) | ||
| self.status.set("New area discovered") | ||
| self.stats.incrementScore() | ||
| self.stats.incrementRoomsExplored() | ||
| else: | ||
| self.currentRoom = room | ||
| room = roomJsonReaderWriter.loadRoom(nextRoomPath) | ||
| self.map.addRoom(room) | ||
| self.status.set("Area loaded") | ||
| return room |
There was a problem hiding this comment.
In _loadOrGenerateRoom, self.map.getRoom(x, y) already loads a room from disk when the room JSON exists (see Map.getRoom). Because of that, the subsequent os.path.exists(nextRoomPath) + manual RoomJsonReaderWriter.loadRoom() block is effectively unreachable, and the "Area loaded" status message will never be shown when loading a saved room. Consider restructuring this helper to either (a) use map.hasRoom() to detect cached rooms and perform the disk-load here (so you can set status), or (b) move the status side-effect into Map.getRoom and remove the redundant disk-load logic here.
| roomX = jsonPlayerLocation["roomX"] | ||
| roomY = jsonPlayerLocation["roomY"] | ||
| currentRoom = mapInstance.getRoom(roomX, roomY) | ||
|
|
||
| locationId = jsonPlayerLocation["locationId"] | ||
| location = currentRoom.getGrid().getLocation(locationId) | ||
| currentRoom.addEntityToLocation(self.player, location) | ||
| return currentRoom |
There was a problem hiding this comment.
loadPlayerLocationFromFile() assumes mapInstance.getRoom(roomX, roomY) always returns a valid Room. If the room file is missing/corrupt and getRoom() returns -1, the next line will raise (no getGrid()). It should guard against -1 (and potentially a missing locationId in the grid) and return None (optionally logging a warning) so the caller can fall back to generating the starting room.
| try: | ||
| configFilePath.write_text("\n".join(newLines) + "\n", encoding="utf-8") | ||
| except OSError: | ||
| pass | ||
| _logger.warning("failed to save window size to config file") |
There was a problem hiding this comment.
The new warning on except OSError drops the underlying exception details and the target path, which makes diagnosing save failures hard. Capture the exception (except OSError as e) and include error=str(e) and the config file path in the warning context.
| try: | ||
| configFilePath.write_text("\n".join(newLines) + "\n", encoding="utf-8") | ||
| except OSError: | ||
| pass | ||
| _logger.warning("failed to save key bindings to config file") |
There was a problem hiding this comment.
The new warning on except OSError omits the exception details and the config file path, which reduces debuggability of persistence failures. Capture the exception (except OSError as e) and log error=str(e) plus the target configFilePath in the warning context.
|
@copilot You are a coding agent tasked with resolving all open comments and review threads Instructions1. Discover all comments
2. For each comment, do the following:
3. Types of issues to handle:
4. Do NOT:
After making changes
Context
|
…Room, guard loadPlayerLocationFromFile against missing room/location, include exception details in OSError warnings Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/cd904318-5d61-4103-b72a-0c69fdb38a9c Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
All four review threads have been addressed in commit d03c1b7:
All 409 tests pass and |
| result = self.persistence.loadPlayerLocationFromFile(self.map) | ||
| if result is not None: | ||
| self.currentRoom = result |
There was a problem hiding this comment.
loadPlayerLocationFromFile() now treats a failed load as result is None (e.g., missing room/location), but initialize() only falls back to spawn when the file is absent. If the file exists but loading fails, WorldScreen.currentRoom may remain unset and subsequent initialization will crash. Consider making this method ensure a valid room/player placement (e.g., spawn fallback) when persistence returns None, or have initialize() handle the None case explicitly.
| def _loadOrGenerateRoom(self, x, y): | ||
| if self.map.hasRoom(x, y): | ||
| return self.map.getRoom(x, y) | ||
| nextRoomPath = self.persistence.buildRoomPath(x, y) | ||
| if os.path.exists(nextRoomPath): | ||
| roomJsonReaderWriter = RoomJsonReaderWriter( | ||
| self.config.gridSize, self.graphik, self.tickCounter, self.config | ||
| ) | ||
| if os.path.exists(nextRoomPath): | ||
| roomJsonReaderWriter = RoomJsonReaderWriter( | ||
| self.config.gridSize, self.graphik, self.tickCounter, self.config | ||
| ) | ||
| room = roomJsonReaderWriter.loadRoom(nextRoomPath) | ||
| self.map.addRoom(room) | ||
| self.currentRoom = room | ||
| self.status.set("Area loaded") | ||
| else: | ||
| x, y = self.getCoordinatesForNewRoomBasedOnPlayerLocationAndDirection() | ||
| self.currentRoom = self.map.generateNewRoom(x, y) | ||
| self.status.set("New area discovered") | ||
| self.stats.incrementScore() | ||
| self.stats.incrementRoomsExplored() | ||
| else: | ||
| self.currentRoom = room | ||
| room = roomJsonReaderWriter.loadRoom(nextRoomPath) | ||
| self.map.addRoom(room) |
There was a problem hiding this comment.
_loadOrGenerateRoom() manually constructs a new RoomJsonReaderWriter and reimplements Map.getRoom()'s load-from-disk behavior. This duplicates existing logic and bypasses the already-resolved self.roomJsonReaderWriter, increasing the chance of behavior drift (e.g., if RoomJsonReaderWriter gains state/config). Prefer delegating to Map.getRoom() (which already loads+adds under the Map lock) and/or using self.roomJsonReaderWriter.loadRoom() rather than instantiating a new reader/writer here.
| locationId = jsonPlayerLocation["locationId"] | ||
| location = currentRoom.getGrid().getLocation(locationId) | ||
| if location is None: | ||
| _logger.warning( | ||
| "saved location not found in room, falling back to spawn", | ||
| locationId=locationId, |
There was a problem hiding this comment.
WorldScreenPersistence.loadPlayerLocationFromFile() calls Grid.getLocation(locationId) and then checks if location is None, but Grid.getLocation() indexes the dict and raises KeyError when the ID is missing (it never returns None). This means corrupted/mismatched location IDs will currently crash before the warning/fallback path. Handle missing IDs via try/except KeyError (or use a safe accessor) and then return None so WorldScreen can fall back to spawn cleanly.
|
@copilot You are a coding agent tasked with resolving all open comments and review threads Instructions1. Discover all comments
2. For each comment, do the following:
3. Types of issues to handle:
4. Do NOT:
After making changes
Context
|
…-load to Map.getRoom(), use try/except KeyError for Grid.getLocation() Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/f372a9ab-2bfb-4fc0-bc55-ae19ea0affc2 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
All three review threads from round 2 addressed in commit c29ccdd:
All 409 tests pass and |
|
@copilot Review code base and ensure that the dependency injection approach using the DI container is being followed when it makes sense and ensure that we are not manually instantiating objects when we should not be |
…mFactory and RoomJsonReaderWriter factory into Map via bootstrap Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/ac4c7133-3d2d-4af8-abdb-bcb5bda08c38 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Audited the codebase for DI violations and fixed 3 issues in commit 46c5ddf:
No other DI violations found — all other
All three review threads from round 2 were already addressed in commit c29ccdd. The subsequent DI audit (commit 46c5ddf) addressed the additional request to ensure proper DI container usage across the codebase. |
Systematic Clean Code refactoring addressing DRY violations, long methods, redundant comments, silent error handling, and DI container compliance across the codebase.
Entity hierarchy DRY
Moved
isSolid()andsolidintoDrawableEntitybase class withsolid=Falsedefault parameter. Eliminated identicalisSolid()/self.solidboilerplate from 20+ entity subclasses.WorldScreen decomposition (2122 → 1871 lines)
WorldScreenPersistence— save/load delegation for player location, attributes, inventory, roomspickupableEntities—canBePickedUp()andPICKUPABLE_TYPEStupledraw()218→52,run()156→16,changeRooms()104→30,handleKeyDownEvent()73→30_drawDayNightOverlay,_drawHotbar,_drawDebugInfo,_processEvents,_updateLivingEntities,_updateGameState,_loadOrGenerateRoom,_calculateTargetLocationForRoomTransition,_handleMovementKey,_handleHotbarKey,_handleUtilityKey,_handleHotbarClick,_handleWorldClick,_plantWheatSeed,_tryHarvestCrop,_drawHotbarSelectionIndicator_checkCooldown(tickToCheck, speed)Screen class DRY
drawButtonblocks with data-driven_toggleConfigAttribute(attributeName)and atoggleButtonslist_switchToScreen(screenType)base method, data-driven menu button renderingComments and error handling
@author/@sinceand why comments_logger.warning()to 2 silentexcept OSError: passblocks in config/keyBindings persistence, including exception details and file path in structured log contexthighestmtps→highestMeasuredTpsDI container audit
Audited the codebase for dependency injection violations and eliminated manual instantiation of DI-registered types:
WorldScreenPersistence: Registered with@componentdecorator and added type hints to all constructor parameters.WorldScreennow resolves it viacontainer.resolve()instead of manual construction.Map→RoomFactory:Map.__init__()now accepts an optionalroomFactoryparameter, injected viabootstrap.py. Falls back to manual construction when not provided (backward compatibility for tests).Map→RoomJsonReaderWriter:Map.__init__()now accepts an optionalroomJsonReaderWriterFactorycallable, injected viabootstrap.pyusing the same factory-callback pattern asRoomPreloader. Eliminates duplicateRoomJsonReaderWriterinstantiation inMap.getRoom().Robustness fixes from review feedback
_loadOrGenerateRoom()delegates disk-load toMap.getRoom()instead of duplicating logicinitialize()handlesNonefromloadPlayerLocationFromFile()with spawn fallbackloadPlayerLocationFromFile()guards againstgetRoom()returning-1and usestry/except KeyErrorforGrid.getLocation()since it raisesKeyErroron missing IDsAll 409 tests pass.
./format.shclean.