py_battleship by sergmark85 · Pull Request #668 · mate-academy/py-battleship

sergmark85 · 2026-03-09T17:20:12Z

No description provided.

mateacademy-ai-mentor

Review Summary

Your implementation of the Battleship game is very solid, and the core logic works correctly. However, a couple of changes are needed before this can be approved.

First, the self.ships attribute in the Battleship class is initialized but never used, which violates one of the project's checklist requirements [CHECKLIST ITEM #4]. Please remove this attribute as it is not being read anywhere.

Additionally, some of your methods have incorrect return type hints. For instance, the fire method returns a string like "Hit!" or "Sunk!", so its return type hint should be str, not Ship. Similarly, another function is hinted to return None but can also return a Deck object, so its type hint should be updated to Deck | None to reflect all possible return values.

Please correct these issues. Great work on the overall structure!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-09T17:21:46Z

app/main.py

+
+        raise ValueError
+
+    def get_deck(self, row: int, column: int) -> None:


The return type hint is None, but the function can return a Deck object on line 31. The type hint should reflect all possible return types, for instance, Deck | None.

mateacademy-ai-mentor · 2026-03-09T17:21:46Z

app/main.py

-        pass
+    def __init__(self, ships: list) -> None:
+        self.field = {}
+        self.ships = []


This attribute self.ships is initialized and populated, but its value is never read or used. This violates checklist item #4: 'Check that all the attributes you define in the __init__ method are used.'

mateacademy-ai-mentor · 2026-03-09T17:21:47Z

app/main.py

+            for deck in ship.decks:
+                self.field[(deck.row, deck.column)] = ship
+
+    def fire(self, location: tuple) -> Ship:


The return type hint Ship is incorrect. According to the task description, this method should return one of the strings: "Miss!", "Hit!", or "Sunk!". The return type should be str.

mateacademy-ai-mentor

Review Summary

Great job addressing most of the feedback from the previous review! You're very close to a complete solution, but a few issues remain that need to be fixed.

Here's what needs your attention:

Unused Attribute: In app/main.py, the is_drowned attribute in the Ship class is assigned but never used. According to the checklist, all attributes must be used, so please remove this one. [CHECKLIST ITEM #4]
Incorrect Type Hints: The return type hints for two methods are incorrect.
- Battleship.fire returns a str ("Miss!", "Hit!", or "Sunk!"), so its type hint should be str.
- Ship._create_decks returns a list of Deck objects, so its type hint should be list[Deck].
Control Flow: The if statement in _create_decks should be an elif because it's part of a chain of mutually exclusive conditions. [CHECKLIST ITEM #2]

Please address these points, and your solution will be ready for approval!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-09T17:35:23Z

app/main.py

+    def __init__(self, start: int, end: int, is_drowned: bool = False) -> None:
+        self.start = start
+        self.end = end
+        self.is_drowned = is_drowned


This attribute is_drowned is assigned a value but is never used anywhere in the code. According to checklist item #4, all attributes defined in __init__ must be used. Please remove this attribute and its assignments if it's not needed.

mateacademy-ai-mentor · 2026-03-09T17:35:23Z

app/main.py

-    def get_deck(self, row, column):
-        # Find the corresponding deck in the list
-        pass
+    def _create_decks(self) -> list | str:


The return type hint for this method seems incorrect. It returns a list of Deck objects, but the hint suggests it could also return a string. Please update the type hint to list or list[Deck] to accurately reflect the return value.

mateacademy-ai-mentor · 2026-03-09T17:35:23Z

app/main.py

+            start, end = sorted([c1, c2])
+            return [Deck(r1, c) for c in range(start, end + 1)]
+
+        if c1 == c2:


This if statement should be an elif. Using a chain of if statements where each block ends with a return is discouraged by checklist item #2, which recommends using an if/elif structure for mutually exclusive conditions.

mateacademy-ai-mentor · 2026-03-09T17:35:23Z

app/main.py

+            for deck in ship.decks:
+                self.field[(deck.row, deck.column)] = ship
+
+    def fire(self, location: tuple) -> str | Ship:


The return type hint for this method is incorrect. The method returns one of the required strings ("Miss!", "Hit!", or "Sunk!"), but never a Ship object. The correct type hint should be str.

mateacademy-ai-mentor

Review Summary

Excellent work on the updates! I am approving this solution as an educational override to help you move forward, as there are still a few outstanding issues from the previous review.

You've done a great job fixing the control flow in _create_decks. For your next submission, please focus on these remaining points:

The is_drowned attribute in the Ship class is still present but unused. Remember to remove unused attributes to keep your code clean [CHECKLIST ITEM #4].
Some return type hints are still incorrect. For instance, get_deck returns a single Deck or None, so its type hint should be Deck | None, not list[Deck].
There's a type hint inconsistency between what Battleship.fire is hinted to return (str) and what Ship.fire can return (None | str). Aligning these makes the code more predictable.

You're doing great—keep up the fantastic effort!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-03-09T17:44:51Z

app/main.py

-    def get_deck(self, row, column):
-        # Find the corresponding deck in the list
-        pass
+    def _create_decks(self) -> list | str:


The return type hint for this method is still incorrect. It should be list[Deck] since it returns a list of Deck objects. This was mentioned in the previous review.

mateacademy-ai-mentor · 2026-03-09T17:44:51Z

app/main.py

+
+        raise ValueError
+
+    def get_deck(self, row: int, column: int) -> list[Deck]:


The return type hint here is incorrect. This method returns a single Deck instance or None, not a list of decks. The type hint should reflect this, for example: Deck | None.

mateacademy-ai-mentor · 2026-03-09T17:44:51Z

app/main.py

+        deck.is_alive = False
+
+        if all(not d.is_alive for d in self.decks):
+            self.is_drowned = True


This violates checklist item #4: 'Remove unused attributes.' The is_drowned attribute is assigned here but never used anywhere in the code. This was also mentioned in the previous review.

mateacademy-ai-mentor · 2026-03-09T17:44:52Z

app/main.py

+            return "Miss!"
+
+        ship = self.field[(row, col)]
+        return ship.fire(row, col)


There's a type hint mismatch here. Battleship.fire is hinted to return str (line 57), but it returns the result of ship.fire(), which is hinted to return None | str (line 34). While the current logic prevents None from being returned, this inconsistency makes the code harder to reason about. Please align the type hints to ensure consistency.

py_battleship

9098404

mateacademy-ai-mentor suggested changes Mar 9, 2026

View reviewed changes

py_battleship2

17430bb

sergmark85 requested a review from mateacademy-ai-mentor March 9, 2026 17:33

mateacademy-ai-mentor suggested changes Mar 9, 2026

View reviewed changes

py_battleship3

023c768

sergmark85 requested a review from mateacademy-ai-mentor March 9, 2026 17:43

mateacademy-ai-mentor approved these changes Mar 9, 2026

View reviewed changes


		raise ValueError

		def get_deck(self, row: int, column: int) -> None:

Conversation

sergmark85 commented Mar 9, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants