Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed for your submission because several critical bugs prevent the game from functioning correctly. You've set up a good class structure, but the implementation details in the Battleship class need some adjustments.
Specifically, please address the following:
- In
Battleship.__init__, you are appending theShipclass toself.shipsinstead of theshipinstance. It should beself.ships.append(ship). - The loop that populates
self.fieldis incorrectly indented and only processes the last ship. It needs to be moved inside thefor start, end in ships:loop. - The
_validate_fieldmethod is never called from__init__. It also contains a logical flaw where a ship's own decks are considered invalid neighbors. - The board update symbols used in the
firemethod do not match the extra task requirements (*for a hit,xfor a sunk ship deck).
Once these issues are resolved, your game should work as expected. You're on the right track!
✨ 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: tuple, | ||
| column: tuple, |
There was a problem hiding this comment.
The type hints for row and column are tuple, but they are initialized with integer values when a Deck instance is created in the Ship class. They should be hinted as int.
| self.is_drowned = True | ||
| return "Sunk!" | ||
|
|
||
| return "Hit!" |
There was a problem hiding this comment.
The return "Hit!" statement is outside the if deck: block. This means if get_deck were to return None (which would signify a miss on this specific ship), this method would still return "Hit!". For better logical clarity, this return statement should be inside the if deck: block.
| # 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.
As per the extra task requirements, the _validate_field method should be called from within __init__ after the field has been created.
| self.field = {} | ||
| for start, end in ships: | ||
| ship = Ship(start, end) | ||
| self.ships.append(Ship) |
There was a problem hiding this comment.
You are appending the Ship class to self.ships, not the ship instance that was just created. This should be self.ships.append(ship).
| for deck in ship.decks: | ||
| self.field[(deck.row, deck.column)] = ship |
There was a problem hiding this comment.
This loop, which populates the self.field dictionary, is incorrectly indented. It is outside the for start, end in ships: loop, which means it will only run once, using the ship variable from the last iteration of that loop. As a result, only the very last ship will be added to the game field. This loop should be indented to be inside the one above it.
| for ship in self.ships: | ||
| for deck in ship.decks: | ||
| for dr in [-1, 0, 1]: | ||
| for dc in [-1, 0, 1]: | ||
| neighbor = (deck.row + dr, deck.column + dc) | ||
| if neighbor in occupied: | ||
| raise AssertionError | ||
| occupied.add((deck.row, deck.column)) |
There was a problem hiding this comment.
The logic for checking if ships are in neighboring cells is flawed. Because you add a deck's coordinates to occupied immediately after checking its neighbors, the check will fail for any multi-deck ship. The second deck of a ship will be considered a neighbor of the first deck, which is already in the occupied set, causing a false AssertionError. The coordinates of all decks of a single ship should be checked against occupied before any of them are added to it.
| if location not in self.field: | ||
| game_desk[location[0]][location[1]] = "*" | ||
| return "Miss!" | ||
|
|
||
| ship = self.field[location] | ||
| hit = ship.fire(location[0], location[1]) | ||
| if hit == "Hit!" or hit == "Sunk!": | ||
| game_desk[location[0]][location[1]] = "X" | ||
| return hit |
There was a problem hiding this comment.
The board update logic does not match the extra task requirements for print_field.
- A miss is marked with
*, but*should be used for a hit deck of an alive ship. - A hit deck is marked with
X. It should be*. - A sunk ship deck is also marked with
X. It should bex, and all decks of the sunk ship should be updated, not just the one that was hit last.
2 participants
No description provided.