Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your Battleship implementation is well-structured and handles most game mechanics effectively, demonstrating a solid understanding of the requirements. However, changes are needed to address a critical inconsistency in symbol usage and some code redundancy. Specifically, the fire method in app/main.py uses 'o' for missed shots, which deviates from the requirement that print_field must use '~' for empty cells, and 'o' is not a defined symbol in the task description. Please ensure that a missed shot on an empty cell leaves the '~' symbol unchanged. Additionally, the Ship.get_deck method in app/main.py is defined but never used, violating the principle that one function should have one purpose [CHECKLIST ITEM #1] and contributing to redundant statements. Finally, consider correcting the type hint for the ships parameter in Battleship.__init__ to reflect that it expects a list of coordinate tuples, not list[Ship]. Once these adjustments are made, your solution will be even stronger.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
app/main.py
Outdated
| @staticmethod | ||
| def get_deck(row: int, column: int) -> Deck: | ||
| return Deck(row, column) |
There was a problem hiding this comment.
The get_deck method in the Ship class is defined but never used. This violates checklist item #1: 'Functions must adhere to the principle that one function has one purpose.' as it's a redundant method, and it also violates 'Do not use redundant statements.'
app/main.py
Outdated
| if result != "Miss!": | ||
| self.field[row][col] = "*" if result == "Hit!" else "x" | ||
| return result | ||
| self.field[row][col] = "o" |
There was a problem hiding this comment.
When a shot misses, the field cell self.field[row][col] is set to "o". The task description specifies "~" for empty cells and does not define "o" for missed shots. A missed shot on an empty cell should ideally leave the "~" symbol unchanged to reflect that it's still an empty cell as per the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core functional requirement is not yet met, which affects the correctness of the game state. At the moment, when a ship is sunk, only the last fired cell is updated to 'x', while the other decks of that same ship stay as '□'; the specification explicitly requires that all decks of a drowned ship be marked with 'x', so your Battleship.fire logic needs to iterate over ship.decks and set self.field[d.row][d.column] = 'x' for each of them when result == "Sunk!". This is a behavioral mismatch that would cause tests or visual checks of the field state to fail and therefore counts as a critical blocker.
The rest of the core functionality is well done: 10x10 field, ship placement from end coordinates, fire returning "Miss!"/"Hit!"/"Sunk!", and validation of ship counts and neighbor rules all look solid. As a non-blocking improvement, consider fixing the Battleship.__init__ type hint so it reflects the actual input type (a list of coordinate tuples rather than list[Ship]), which will make the API clearer and avoid confusion for readers or type-checking tools. Once you adjust the sunk-ship marking so that every deck is turned into 'x', your solution should fully satisfy the specification. Keep up the good work—you're very close; this is mainly a small but important correctness detail to fix.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
app/main.py
Outdated
| def fire(self, location: tuple) -> str: | ||
| row, col = location | ||
| for ship in self.ships: | ||
| result = ship.fire(*location) | ||
| if result != "Miss!": | ||
| self.field[row][col] = "*" if result == "Hit!" else "x" | ||
| return result | ||
| # self.field[row][col] = "o" |
There was a problem hiding this comment.
When a ship is sunk you only set the fired cell in the field to 'x'. The extra-task/specification requires using x for decks of the drowned ship (all decks). Update the logic so that when result == "Sunk!" you iterate over ship.decks and set every corresponding self.field[d.row][d.column] = 'x' (replace any prior '*' or '□' for that ship).
app/main.py
Outdated
| # If it is, then it should check if this cell is the last alive | ||
| # in the ship or not. | ||
| pass | ||
| def __init__(self, ships: list[Ship]) -> None: |
There was a problem hiding this comment.
Type hint mismatch: __init__ is annotated ships: list[Ship] but the constructor actually expects a list of coordinate tuples like ((0,0),(0,3)). Consider changing the annotation to reflect that (for example ships: list[tuple[tuple[int, int], tuple[int, int]]] or a simpler list[tuple]) to avoid confusion.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed the critical issue from the previous review, and the logic for marking sunk ships is now correct. I am approving your solution as an educational override so you can continue to the next stage.
There is one minor point for improvement: the self.is_drowned attribute in the Ship class (app/main.py) is assigned but never used. Since the "sunk" status is determined dynamically, this attribute is redundant and can be removed to make the code cleaner. [CHECKLIST ITEM #4]
Great job getting the functionality right. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| end: tuple, | ||
| is_drowned: bool = False | ||
| ) -> None: | ||
| self.is_drowned = is_drowned |
There was a problem hiding this comment.
This attribute self.is_drowned is assigned a value here and on line 45, but it's never read or used anywhere else in the program. This violates checklist item #4: 'Remove unused attributes.' The state of the ship (whether it's sunk or not) is determined on-the-fly by checking its decks, so this attribute is redundant. Consider removing it, along with the is_drowned parameter from the __init__ method.
2 participants
No description provided.