Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Develop #491

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Develop #491

wants to merge 6 commits into from

Conversation

dmytrik
Copy link

@dmytrik dmytrik commented Oct 14, 2024

No description provided.

app/main.py Outdated
Comment on lines 59 to 60
for i in range(start[1], end[1] + 1):
result.append(Deck(start[0], i))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use more descriptive variable. column, for example or row in other case

# Create a dict `self.field`.
# Its keys are tuples - the coordinates of the non-empty cells,
# A value for each cell is a reference to the ship
# which is located in it
pass
self.ships = {coords: Ship(coords[0], coords[1]) for coords in ships}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using dict here? Almost in all other methods you are using .values() to get data. So why can't we create simple list?

P.S. I'm not saying that you're wrong, you can prove your point and argue for using dict

Copy link
Author

@dmytrik dmytrik Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

task's condition says to use dictionary and keys help me in method where i find all cells around the ship in which other ships can't be located

app/main.py Outdated
Comment on lines 105 to 139
def print_field(self) -> None:
decks = []
for ship in self.ships.values():
if ship.is_drowned:
for deck in ship.decks:
deck.symbol = "x"
decks.append(deck)
else:
for deck in ship.decks:
if deck.is_alive:
deck.symbol = "\u25A1"
if not deck.is_alive:
deck.symbol = "*"
decks.append(deck)
symbols_list = []
is_found = False
for row, column in self.field:
for index, deck in enumerate(decks):
if deck.row == row and deck.column == column:
symbols_list.append(deck.symbol)
is_found = True
break
if index == len(decks) - 1:
is_found = False

if not is_found:
symbols_list.append("~")

symbols = ""
for index, symbol in enumerate(symbols_list):
if index % 10 == 0 and index != 0:
symbols += "\n"
symbols += f"{symbol} "

print(symbols)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to read this method. We should simplify it.

  1. Let's first create a 10x10 grid initialized with water symbols(~)
  2. Then fill the grid with ship symbols(+ check their status)
  3. Create single string of symbols

app/main.py Outdated
Comment on lines 145 to 150
condition = (0 <= x_[0] < 10
and 0 <= x_[1] < 10
and 0 <= y_[0] < 10
and 0 <= y_[1] < 10)
if not condition:
raise VisibilityError

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor using all

app/main.py Outdated
Comment on lines 153 to 167
one_decks = 0
two_decks = 0
three_decks = 0
four_decks = 0
ships = [ship.decks for ship in self.ships.values()]

for ship in ships:
if len(ship) == 1:
one_decks += 1
if len(ship) == 2:
two_decks += 1
if len(ship) == 3:
three_decks += 1
if len(ship) == 4:
four_decks += 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can create it as a dictionary {1(first deck): 0, 2(second deck): 0}

app/main.py Outdated
Comment on lines 173 to 176
invalid_cells = []
for coord in coords:
cells = self._get_invalid_cells_for_one_ship(coord)
invalid_cells += cells

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use set comprehension here


def _get_invalid_cells_for_one_ship(
self,
coord: tuple

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more descriptive annotation, check all funcs and methods

@dmytrik dmytrik requested a review from AnyoneClown October 17, 2024 16:02
Copy link

@Arsen-hrynevych Arsen-hrynevych left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly correct, just needs a few adjustments and it'll be ok.

if current_ship and not current_ship.is_drowned:
return "Hit!"
if current_ship and current_ship.is_drowned:
return "Sunk!"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After hitting a ship's deck, you should check whether all decks of that ship are "sunk" (i.e., is_alive = False) to return "Sunk!".

current_ship = ship

if current_ship and not current_ship.is_drowned:
return "Hit!"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If other decks of the ship are still alive, return "Hit!".

app/main.py Outdated
return "Hit!"
if current_ship and current_ship.is_drowned:
return "Sunk!"
return "Miss!"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no ship exists at the fired location, return "Miss!"

for row in sea:
print(" ".join(row))

def _validate_field(self) -> None:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that exactly 10 ships exist, with the correct number of ships of different sizes (1, 2, 3, and 4 decks).
No two ships should be adjacent, even diagonally.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do this check in the init method after creating the self.ships field as specified in the task condition. Logically, if any of the validation conditions is not met, an error will occur and we will not be able to access this method as well as others.

app/main.py Outdated
for cell in invalid_cells:
for deck in decks:
if cell[0] == deck.row and cell[1] == deck.column:
raise Exception("ships shouldn't be located in"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could create your own Exception

Copy link

@Arsen-hrynevych Arsen-hrynevych left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants