Conversation
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 a couple of required validation features are missing and these are considered blocking for this task. The Battleship class is supposed to define and call a protected _validate_field method from __init__, but this method is not implemented or invoked, so fleet composition (ship counts by size) and adjacency rules (no touching, even diagonally) are not enforced. In app/main.py, before writing to self.field[(deck.row, deck.column)], you need to validate that each coordinate is within the 10x10 bounds (rows and columns in 0..9) and raise an error if any deck is out of range, and you also need to detect and reject overlapping ships when a cell is already occupied. Additionally, Ship.__init__ currently assigns self.start and self.end but never uses them, which violates [CHECKLIST ITEM #4]; either remove these attributes or make them part of your validation/representation logic. To move this forward, please: (1) implement _validate_field to check ship counts and ensure ships do not neighbor (even diagonally), (2) call _validate_field at the end of Battleship.__init__, (3) add bounds and overlap checks when building self.field, and (4) clean up or use the unused attributes. Once these are addressed, the otherwise solid Deck/Ship modeling and fire logic will fully meet the task 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
| end: Tuple[int, int], | ||
| is_drowned: bool = False | ||
| ) -> None: | ||
| self.start = start |
There was a problem hiding this comment.
This violates CheckList item #4: "Remove unused attributes. Check that all the attributes you define in the __init__ method are used." You assign self.start here but that attribute is never used elsewhere; either use it (e.g., for validation/representation) or remove it.
app/main.py
Outdated
| is_drowned: bool = False | ||
| ) -> None: | ||
| self.start = start | ||
| self.end = end |
There was a problem hiding this comment.
This violates CheckList item #4: "Remove unused attributes. Check that all the attributes you define in the __init__ method are used." You assign self.end here but that attribute is never used elsewhere; either use it (e.g., for validation/representation) or remove it.
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[Tuple[Tuple[int, int], Tuple[int, int]]]) -> None: |
There was a problem hiding this comment.
The task requires a protected _validate_field method that should be called after creating the field in __init__. I don't see _validate_field implemented or invoked — please implement _validate_field and call it at the end of Battleship.init to enforce the required checks (total ships, counts by size, and no neighboring ships) as described in the task.
| def __init__(self, ships: List[Tuple[Tuple[int, int], Tuple[int, int]]]) -> None: | ||
| self.field: Dict[Tuple[int, int], Ship] = {} | ||
|
|
||
| for start, end in ships: |
There was a problem hiding this comment.
Before inserting ship decks into self.field, validate input coordinates are within the 10x10 bounds (rows and columns must be in 0..9). The description defines the field as a 10x10 matrix; add range checks and raise an error for out-of-range coordinates.
| for start, end in ships: | ||
| new_ship = Ship(start, end) | ||
| for deck in new_ship.decks: | ||
| self.field[(deck.row, deck.column)] = new_ship |
There was a problem hiding this comment.
When assigning self.field[(deck.row, deck.column)] = new_ship you should check for existing entries at that coordinate. If a deck coordinate is already present, the input describes overlapping ships which should be considered invalid — detect and raise an error (this also helps when implementing _validate_field).
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 there is a functional gap in your ship-construction logic that conflicts with the problem statement. Right now, Ship.__init__ builds all cells in the full rectangle between start and end (e.g., from (0,0) to (1,1) you create a 2x2 square of 4 cells), which allows L-shaped or block-shaped ships. According to the task, ships are defined as continuous straight lines by their end coordinates and must be either purely horizontal or purely vertical, so both row and column ranges cannot be greater than zero at the same time.
You should update Ship.__init__ to validate orientation: if start[0] != end[0] and start[1] != end[1], raise a ValueError to reject that ship definition and avoid building a rectangular area. After that check, construct decks only along a single row (horizontal) or a single column (vertical), as intended. As a minor style improvement, you can also remove the explicit return None at the end of Ship.get_deck, since it is redundant, but this is not critical. Once the ship-orientation validation is in place, your solution will align well with the task’s rules and your other logic (bounds, overlap, adjacency, and fleet composition) is already solid.
✨ 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
| row_start, row_end = min(start[0], end[0]), max(start[0], end[0]) | ||
| col_start, col_end = min(start[1], end[1]), max(start[1], end[1]) | ||
|
|
||
| def get_deck(self, row, column): | ||
| # Find the corresponding deck in the list | ||
| pass | ||
| for row_idx in range(row_start, row_end + 1): | ||
| for col_idx in range(col_start, col_end + 1): | ||
| self.decks.append(Deck(row_idx, col_idx)) |
There was a problem hiding this comment.
Ship.init builds all cells in the rectangular area between start and end (a cartesian product of rows and columns). This allows non-linear/rectangular ships (e.g., start=(0,0), end=(1,1) -> 4-cell square). The task examples and description imply ships should be continuous straight lines (horizontal or vertical) defined by their end coordinates — see: "We will define a ship by the coordinates of its ends... so a tuple ((0, 0), (0, 3)) corresponds to it." . Please validate orientation and reject ship definitions where both row and column ranges are > 0 (i.e., require start[0] == end[0] or start[1] == end[1]) and raise a ValueError for invalid ship shapes.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you polish the remaining minor detail. Functionally, your Battleship and Ship implementations are strong: you correctly build the 10x10 field, enforce straight-line ships, prevent overlaps and adjacency, and return the right messages for fire() depending on hits, misses, and sunk ships. One minor style issue remains in app/main.py: in Ship.get_deck (near the end of that method), you have a final return None that is never needed because Python already returns None implicitly when the function reaches the end without a return value — this conflicts with [CHECKLIST ITEM #2]. To improve, simply remove that last return None so the function ends naturally after the loop. Overall, your code quality and logic are solid; this is now just about aligning with clean-code style guidelines, which you’re very close to doing already.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| for deck in self.decks: | ||
| if deck.row == row and deck.column == column: | ||
| return deck | ||
| return None |
There was a problem hiding this comment.
This violates checklist item #2: "Don't use a redundant return statement if it doesn't do anything." The final return None in Ship.get_deck is redundant — remove it and let the function implicitly return None when no deck is found.
2 participants
No description provided.