Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good job on the overall structure with the Deck, Ship, and Battleship classes! However, this requires changes because there are a few critical issues with the game logic that need to be addressed.
Here are the main points to fix:
- Incorrect Ship Creation: The current logic can create rectangular ships (e.g., from
(0,0)to(1,1)). Ships must be a straight line, either horizontally or vertically. Please adjust your logic to create decks only along a single row or column. - Handling Repeated Shots: The
firemethod returnsNonefor a shot on an already-hit cell ('*'or'X'). The method must always return one of the required strings ("Miss!","Hit!", or"Sunk!"). A repeated shot should probably be considered a"Miss!". - Incomplete Board Update: When a ship is sunk, all of its decks must be updated to
'X'on the game field. Currently, only the last-hit deck is updated, while the others remain as'*'. - Redundant
else: Theelsestatement in yourfiremethod is redundant because the precedingifblock has areturn. This violates a checklist item [CHECKLIST ITEM #3].
Please fix these issues to ensure the game functions correctly according to the requirements.
✨ 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
| for row in range(row_start, row_end + 1): | ||
| for column in range(column_start, column_end + 1): | ||
| self.decks.append(Deck(row, column)) |
There was a problem hiding this comment.
The current logic for creating ship decks can result in rectangular ships (e.g., for start=(0,0) and end=(1,1)), which is not standard for Battleship. Ships should always be straight lines, either horizontal or vertical. You should adjust this logic to create decks only along a single row or a single column.
app/main.py
Outdated
| if ship.is_drowned: | ||
| self.field[row][column] = "X" | ||
| return "Sunk!" |
There was a problem hiding this comment.
When a ship is sunk, you should update all its decks on the game field to reflect this status (e.g., to 'X'). Currently, only the deck that was last hit is being updated, leaving other parts of the sunk ship marked as just 'hit' ('*').
app/main.py
Outdated
| else: | ||
| self.field[row][column] = "*" | ||
| return "Hit!" |
There was a problem hiding this comment.
This else statement is redundant. The if block above it contains a return, so the code inside this else will only execute if the if condition is false anyway. You can remove the else and unindent its content. This violates checklist item #3: 'Don't use redundant else if a non-empty return is used'.
app/main.py
Outdated
| else: | ||
| self.field[row][column] = "*" | ||
| return "Hit!" | ||
| return None |
There was a problem hiding this comment.
The method returns None if a shot is fired at a cell that has already been hit (marked with '*' or 'X'). The task description requires the method to return one of the specified strings ("Miss!", "Hit!", or "Sunk!"). Firing at an already-hit cell should probably be considered a "Miss!".
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job fixing the critical issues from the last review! The core logic for handling ships and shots is working perfectly now. However, this submission requires changes because the two extra methods from the task description, print_field and _validate_field, have not been implemented yet. While they were marked as optional, they are part of the overall requirements for completing this task. Please add these methods. Also, consider updating the fire method's return type hint from str | None to str for better accuracy, as it never returns None. You're very close to finishing this!
✨ 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
| self.is_drowned = all(not d.is_alive for d in self.decks) | ||
|
|
||
|
|
||
| class Battleship: |
There was a problem hiding this comment.
The two extra tasks from the description, print_field and _validate_field, have not been implemented. While they are optional for the core functionality, they are part of the overall task requirements.
app/main.py
Outdated
| for deck in new_ship.decks: | ||
| self.field[deck.row][deck.column] = "□" | ||
|
|
||
| def fire(self, location: tuple) -> str | None: |
There was a problem hiding this comment.
The return type hint str | None is not entirely accurate. This method always returns a string ("Miss!", "Hit!", or "Sunk!"), so it never returns None. The type hint should be simplified to -> str.
|
|
||
| if ship.is_drowned: | ||
| for deck in ship.decks: | ||
| self.field[deck.row][deck.column] = "X" |
There was a problem hiding this comment.
The task description for the print_field method specifies using a lowercase 'x' for the decks of a sunk ship. Using 'X' here is inconsistent with that requirement. This also affects the check on line 50.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the print_field and _validate_field methods! The logic you've written is solid and you're very close to a perfect solution.
I am approving this submission as an educational override to help you move forward, but there is one crucial detail to address. While you've correctly implemented the _validate_field method, it is never actually called. To ensure the initial ship placement is always validated when a Battleship object is created, you need to call this method from within your __init__ constructor.
Simply add self._validate_field() at the end of the __init__ method. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| def __init__(self, ships: list) -> None: | ||
| self.field = [["~" for _ in range(10)] for _ in range(10)] | ||
| self.ships_objects = [] | ||
|
|
||
| for start, end in ships: | ||
| new_ship = Ship(start, end) | ||
| self.ships_objects.append(new_ship) | ||
|
|
||
| for deck in new_ship.decks: | ||
| self.field[deck.row][deck.column] = "□" |
There was a problem hiding this comment.
The task requires _validate_field to be called from the __init__ method. You've correctly implemented the validation logic, but it needs to be invoked here after all ships have been processed to ensure the initial field is valid.
2 participants
No description provided.