Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the overall structure and the core logic for the fire method! However, I'm requesting a few changes before this can be approved. There are two critical issues that are blocking the solution: first, the required print_field and _validate_field methods are missing from the Battleship class. Second, there is a bug in the Ship constructor that prevents horizontal ships from being created correctly; a ship is horizontal when self.start_y == self.end_y. Please also address the unused attributes noted in the comments, as this violates a checklist item [CHECKLIST ITEM #4]. Once these issues are resolved, your solution will be ready for approval!
✨ 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
| self.start_x, self.start_y = start | ||
| self.end_x, self.end_y = end |
There was a problem hiding this comment.
These attributes (start_x, start_y, end_x, end_y) are only used within the __init__ method to create the decks. They are not used anywhere else in the Ship class. This violates checklist item #4: 'Remove unused attributes.' Consider using local variables within __init__ instead of instance attributes.
app/main.py
Outdated
| # Change the `is_alive` status of the deck | ||
| # And update the `is_drowned` value if it's needed | ||
| pass | ||
| elif self.start_y < self.end_y: |
There was a problem hiding this comment.
This condition for identifying a horizontal ship is incorrect. A ship is horizontal if its start and end y-coordinates are the same. This condition should check for equality (self.start_y == self.end_y) to correctly handle horizontal ships.
app/main.py
Outdated
| else: | ||
| return |
There was a problem hiding this comment.
This block violates checklist item #2. An __init__ method should not fail silently by returning. This creates an object in an invalid state (a ship with no decks). Instead, you should raise an exception (e.g., ValueError) for invalid ship coordinates like diagonal ones.
| self.is_drowned = True | ||
|
|
||
|
|
||
| class Battleship: |
There was a problem hiding this comment.
The Battleship class is missing the print_field and _validate_field methods, which are required by the 'Extra tasks' section in the task description. Please implement these methods to add the required printing and validation functionality.
| self, | ||
| ships: list | ||
| ) -> None: | ||
| self.ships = [] |
There was a problem hiding this comment.
The self.ships attribute is populated in the __init__ method but is never used afterward. This violates checklist item #4: 'Remove unused attributes.' A local variable within __init__ would be more appropriate for building the ships before populating self.field.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the core game logic and fixing the previous bugs! Your solution is very close, but it requires a few changes before it can be approved.
Here are the main points to address:
- The
_validate_fieldmethod is correctly implemented, but it's not being called from theBattleship.__init__method. Please add a call toself._validate_field()at the end of the constructor to ensure the game board is always valid. - Your validation logic is missing a check for the total number of ships. The rules require exactly 10 ships in total, so please add this check to
_validate_field. - In the
Shipclass, several attributes (start_x,start_y, etc.) are only used inside the__init__method. These should be converted to local variables to comply with the project checklist [CHECKLIST ITEM #4].
Once these issues are resolved, your solution will be ready!
✨ 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
| self.start_x, self.start_y = start | ||
| self.end_x, self.end_y = end |
There was a problem hiding this comment.
These attributes (start_x, start_y, end_x, end_y) are only used within the __init__ method to create the decks. They are not used by any other methods in the Ship class. This violates checklist item #4: 'Remove unused attributes.' Consider making them local variables within this method.
| def __init__( | ||
| self, | ||
| ships: list | ||
| ) -> None: | ||
| self.ships = [] | ||
| self.field = {} | ||
| for start, end in ships: | ||
| new_ship = Ship(start, end) | ||
| self.ships.append(new_ship) | ||
|
|
||
| for ship in self.ships: | ||
| for deck in ship.decks: | ||
| coord = (deck.row, deck.column) | ||
| self.field[coord] = ship |
There was a problem hiding this comment.
The _validate_field method is correctly implemented, but it's not being called from within this __init__ method. According to the requirements, you need to call self._validate_field() at the end of this method, after the field has been fully initialized.
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 submission! You've done a great job fixing the issues from the previous review, especially with calling the validation method from the constructor and refactoring the Ship class.
I am approving this solution as an educational override to help you progress, but there is one important validation check still missing. Your _validate_field method doesn't verify that the total number of ships is exactly 10. While you correctly check the counts for each ship size, an invalid field with more than 10 ships could still pass validation.
You can easily fix this by adding a check for the length of the ships list. Keep up the great work, you're very close to a perfect solution!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (lengths.count(4) != 1 or lengths.count(3) != 2 | ||
| or lengths.count(2) != 3 or lengths.count(1) != 4): |
There was a problem hiding this comment.
This validation is still not quite complete. According to the requirements, you need to check that the total number of ships is exactly 10. The current logic only validates the counts of specific ship sizes, which means an invalid field with more than 10 ships could still pass validation.
Please add a check for the total number of ships, for example by checking the length of the lengths list.
2 participants
No description provided.