Conversation
typo on CardinalDirections in RedirectorTile.
…ontrols for one colour
…OS. Change entry point to use hyphen
…config file. Changed characters in manifest to work on windows.
Fix some logging messages
Updated story manifest and added 7 levels to the game.
There was a problem hiding this comment.
README.md
README is descriptive, and installation instructions are correct. Not much else to say, it's a README after all, hehe.
Code quality
The structure of the application is great. Not much I would change, all of the components are logically split up into their respective files and directories.
Flake8 obedience is great, no errors, but there are several blocks of code where I would add more line separation.
The biggest problem to me was the inconsistent docstrings. Some functions and methods have them, but others don't. The ones that don't have docstrings aren't necessarily self-explanatory, either.
Commit Quality
Commits are good, titles are nice and descriptive. However, some commit titles are a bit lengthy in my opinion, and some of that length could be delegated to the commit body instead. Overall, I would be happy if I were a maintainer.
Things You Did Well
- Good code structure
- Type hints everywhere, awesome
- Great commit history
- Great Flake8 compliance
Things that could be improved
- Several misleading method/attribute names
- Several instances of long if statement chains that could be delegated to dictionaries
- Formatting is sometimes subpar
Final words
The game concept, as well as the gameplay itself, is very unique! The code behind it is overall, great. There are changes I would make, but they don't concern any fundamental aspects of the application itself. Good job!
| - Ensures only one instance of each module exists | ||
| - Improve render loop so only updated elements are re-rendered to decrease load | ||
| - Add trap tiles | ||
| - Mutators with positive effect |
There was a problem hiding this comment.
README looks great, installation instructions work as expected.
| def get_board(self) -> List[List[BaseTile]]: | ||
| """Return list of visible tiles within FOV.""" | ||
| ball = self._board.ball | ||
| all_tiles = [row[:] for row in self._board.all_tiles] | ||
| for row in all_tiles: | ||
| for x, tile in enumerate(row): | ||
| if isinstance(tile, GoalTile): | ||
| continue | ||
| if ball.calc_distance(tile) > self.FOV: | ||
| row[x] = BlindTile(pos=(tile.pos.x, tile.pos.y)) | ||
| return all_tiles |
There was a problem hiding this comment.
I think this function should be renamed. It's easy to get confused by the board property and the get_board method, as both seem to imply that they do the same thing but in fact the get_board method returns a list of tiles instead.
In fact, the docstring states that this function returns the visible tiles with respect to the FOV, so I think an appropriate name would be get_visible_tiles.
| if not self._board: | ||
| return |
There was a problem hiding this comment.
This pattern is common throughout other methods in this class. Instead of replicating this code, you could put it into a decorator with a descriptive name such as requires_board.
| def _is_ball_on_goal(self, under_ball: BaseTile) -> None: | ||
| """Test if the ball is on the goal tile""" | ||
| if isinstance(under_ball, GoalTile): | ||
| self.win_count += 1 | ||
| self.emit_event(VictoryEvent(self.win_count)) | ||
| self.new_level() | ||
|
|
||
| def _is_ball_on_story_tile(self, under_ball: BaseTile) -> None: | ||
| if isinstance(under_ball, StoryTile): | ||
| if not under_ball.visited: | ||
| self.emit_event(StoryEvent(self._board.level_name)) | ||
| under_ball.visited = True |
There was a problem hiding this comment.
These function names are misleading. These should only return booleans, separate event handler functions should be utilized instead.
| if ball.direction == CardinalDirection.up: | ||
| y = -1 | ||
| elif ball.direction == CardinalDirection.right: | ||
| x = 1 | ||
| elif ball.direction == CardinalDirection.down: | ||
| y = 1 | ||
| elif ball.direction == CardinalDirection.left: | ||
| x = -1 |
There was a problem hiding this comment.
Instead of these long if statements, a dictionary should be utilized:
example_dict = {
CardinalDirection.up: (0, -1),
CardinalDirection.right: (1, 0),
CardinalDirection.down: (0, 1),
CardinalDirection.left: (-1, 0)
}
x, y = example_dict[ball.direction]| class FOVReduce(BaseMutator): | ||
| """Reduce the field of vision""" | ||
|
|
||
| value = 2 |
There was a problem hiding this comment.
Constants should be in SCREAMING_SNAKE_CASE. value is also a bit of a not-so-descriptive name, something like FOV_STEP would be much better.
| def level_high_score(self) -> str: | ||
| """ | ||
| Return the high score for this level | ||
|
|
||
| If no high score is found for this level return '-' | ||
| """ | ||
| if self._current_level_name in self.high_scores: | ||
| return str(self.high_scores[self._current_level_name]) | ||
| return "-" |
There was a problem hiding this comment.
Function is doing too much by returning "-". Return None, and leave the rest of the logic to the frontend.
| class BallTile(BaseTile): | ||
| """The ball tile""" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.direction = CardinalDirection.down | ||
|
|
||
| def __str__(self): | ||
| return " ○ " | ||
|
|
||
|
|
||
| class GoalTile(BaseTile): | ||
| """Tiles the Goal""" | ||
|
|
||
| def __str__(self): | ||
| return " ■ " | ||
|
|
||
|
|
||
| class PathTile(BaseTile): | ||
| """Tiles the ball can travel across""" | ||
|
|
||
| def __str__(self): | ||
| return " " | ||
|
|
||
|
|
||
| class StoryTile(PathTile): | ||
| """Tiles that the ball can travel across and move the story forward""" | ||
|
|
||
| def __init__(self, pos: Tuple[int, int], color: Tuple[int, int, int]): | ||
| super().__init__(pos, color) | ||
| self.visited = False | ||
|
|
||
| def __str__(self): | ||
| return " ░ " | ||
|
|
||
|
|
||
| class BlindTile(BaseTile): | ||
| """Tiles that are outside of vision.""" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs, color=(32, 32, 32)) | ||
|
|
||
| def __str__(self): | ||
| return " " | ||
|
|
||
|
|
||
| class PauseTile(BaseTile): | ||
| """Tile to show game pause state.""" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs, color=(255, 255, 255)) | ||
|
|
||
| def __str__(self): | ||
| return " \u23F8 " |
There was a problem hiding this comment.
All these __str__ methods should be left to the frontend! The backend should not decide the representation of these tiles.
| from rich.align import Align | ||
| from rich.color import Color | ||
| from rich.layout import Layout | ||
| from rich.live import Live | ||
| from rich.panel import Panel | ||
| from rich.table import Table | ||
| from rich.text import Text | ||
| from rich.tree import Tree | ||
|
|
||
| from pantheras_box.backend.core import CoreBackend | ||
| from pantheras_box.backend.events import BaseEvent, EventTypes | ||
| from pantheras_box.backend.mutators import CoreMutators | ||
| from pantheras_box.backend.scoring import CoreScoring | ||
| from pantheras_box.backend.tiles import PauseTile | ||
| from pantheras_box.keyboard_handlers.core import BaseKeyboardHandler, KeyboardFactory | ||
| from pantheras_box.sounds.core import CoreSounds | ||
| from pantheras_box.story.core import CoreStory |
| ] | ||
| for row in all_tiles: | ||
| table.add_row( | ||
| *[f"[{Color.from_rgb(*tile.color).name}]{tile}" for tile in row] |
There was a problem hiding this comment.
This is a lot of logic in one line. Split it into readable chunks!
No description provided.