Skip to content

Conversation

Copy link

@hayashi-ay hayashi-ay left a comment

Choose a reason for hiding this comment

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

コメントしました。


class Solution:
def maxAreaOfIsland(self, grid: List[List[int]]) -> int:
water = 0

Choose a reason for hiding this comment

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

定数は他の変数と区別する意味でも大文字が良いと思います。
https://google.github.io/styleguide/pyguide.html#3164-guidelines-derived-from-guidos-recommendations

Comment on lines 7 to 8
width = len(grid[0])
height = len(grid)

Choose a reason for hiding this comment

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

これは好み?アクセスする順番は len(grid), len(grid[0])の順番が自然な気がします。

height = len(grid)

def calc_area(x: int, y: int) -> int:
if x >= width or 0 > x or y >= height or 0 > y or visited[y][x] or grid[y][x] == water:

Choose a reason for hiding this comment

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

pythonは比較演算子をchainできるので、not (0 <= x < width)とした方が分かりやすいように思います。

max_area = 0
for y in range(height):
for x in range(width):
visited = [[False] * width for _ in range(height)]

Choose a reason for hiding this comment

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

visitedは2重ループの外で定義するのが良いと思います。今の処理だと一度訪れたislandについても次のループで訪れていると思います。

def find(self, x):
if self.parents[x] == x:
return x
else:

Choose a reason for hiding this comment

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

ifでreturnしているのでここのelseは不要です。

Comment on lines 18 to 19
x = self.find(x)
y = self.find(y)

Choose a reason for hiding this comment

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

変数名をもう少し工夫した方が良いと思います。あと引数を使い回しているのも気になりました。

Copy link
Owner Author

Choose a reason for hiding this comment

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

target1, target2のようにしてみました。hayashiさんのコードではnodeという単語を使っているんですね。union-findがグラフになっているという感覚があまりなくてnodeが出てこなかったです。

if grid[y][x] == land:
areas[uf.find(to_1d(x, y))] += 1

return max([0, *areas.values()])

Choose a reason for hiding this comment

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

こちらの方が自然ですかね? max(areas.values(), default=0)
個人的にはif文を使ってあげるので良いと思います。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。maxにdefaultやkeyを渡せるんですね。
指摘いただいて気づいたのですが、自分の好みとして今回のareasのkeyが無い場合のように相対的に珍しいケースのためにコードを長くしたくないという気持ちがありそうです。

def to_1d(x, y):
return width * y + x

def union_connected_lands(x, y):

Choose a reason for hiding this comment

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

なんかこの関数読むの大変な気がします。

Copy link
Owner Author

Choose a reason for hiding this comment

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

条件式が長いのが原因かなと思い、is_landとare_same_groupという関数を切り出してみました。

Yuki Michishita added 2 commits April 8, 2024 21:32
area = 0
visited = [[False] * width for _ in range(height)]

def search_land(x: int, y: int):
Copy link

Choose a reason for hiding this comment

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

個人的には nonlocal 好きではないので、
area = 1
area += search_land()
area += search_land()
area += search_land()
area += search_land()
return area
としたいですね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

ありがとうございます。変更しました。

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