Skip to content

Conversation

@tarinaihitori
Copy link
Owner

Comment on lines +99 to +100
num_of_rows = len(grid)
num_of_cols = len(grid[0])
Copy link

Choose a reason for hiding this comment

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

この2行は上にあったほうがいいですかね。だいたい上から読んでいくので、初出の際は、分かっているものから繋がっていたほうが分かりやすいです。

return 0
grid[row][col] = VISITED
area = 1
for i, j in DIRECTIONS:
Copy link

Choose a reason for hiding this comment

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

方向を i, j で表している点にやや違和感を感じました。スコープが短いのでギリギリわかるのですが…。step2 以降のように、 delta_row, delta_col と書いたほうが分かりやすいと思います。


deque を使った BFS
破壊的な方法
時間計算量:O(n _ m)
Copy link

Choose a reason for hiding this comment

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

O(n * m) でしょうか…?

Copy link
Owner Author

Choose a reason for hiding this comment

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

すみません変な値になっていました。O(n * m)が正しいです。

VISITED = -1
DIRECTIONS = [(1,0), (0, 1),(-1, 0), (0, -1)]

def calculate_island_area(row, col):
Copy link

Choose a reason for hiding this comment

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

calculate は、計算式を用いて何らかの値を数値計算するというニュアンスを感じます。他の動詞は考えられますか?

Copy link
Owner Author

Choose a reason for hiding this comment

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

そうですね。島を探索しているので「explore_island」か、
島の面積を取得していることに注目すれば「get_island_area」とかでしょうか。

grid[row][col] = VISITED
area = 1
for i, j in DIRECTIONS:
area += calculate_island_area(row + i, col + j)
Copy link

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.

コメントには書いてないですが、スタックオーバーフローは考慮しました。
m,nが最大で50なので50*50で2500回呼び出される可能性があります。
Pythonのデフォルトの再帰の深さは1000なのですが、LeetCodeでは再帰の深さが55,000に設定されているので、
スタックオーバーフローしないことを確認して、再帰を書きました。
ただ、前回も似たような問題で横着してその旨を書いていなかったので今後はちゃんとコメントに書くようにします。

def calculate_island_area(i, j):
islands_to_visit = deque([(i, j)])
area = 0
while islands_to_visit:
Copy link

Choose a reason for hiding this comment

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

スペースは 1 つのみにすることをおすすめいたします。

islands_to_visit = deque([(i, j)])
area = 0
while islands_to_visit:
row, col = islands_to_visit.popleft()
Copy link

Choose a reason for hiding this comment

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

こちらもスペースは 1 つのみにすることをおすすめいたします。

area = 0
while islands_to_visit:
row, col = islands_to_visit.popleft()
if not(0 <= row < num_of_rows and 0 <= col < num_of_cols):
Copy link

Choose a reason for hiding this comment

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

step1 のように not のあとにスペースを空けることをおすすめいたします。

Comment on lines +122 to +123
islands_to_visit = []
islands_to_visit.append((i, j))
Copy link

Choose a reason for hiding this comment

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

islands_to_visit = [(i, j)] のほうがシンプルだと思います。

grid[row][col] = VISITED
area = 1
for i, j in DIRECTIONS:
area += calculate_island_area(row + i, col + j)
Copy link

Choose a reason for hiding this comment

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

個人的には無駄な関数呼び出しを省くため、このタイミングで

if not (0 <= row < num_of_rows and 0 <= col < num_of_cols):
    return 0
if grid[row][col] != LAND:
    return 0

としたほうがよいと思います。

area = 0
while islands_to_visit:
row, col = islands_to_visit.pop()
if not(0 <= row < num_of_rows and 0 <= col < num_of_cols):
Copy link

Choose a reason for hiding this comment

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

not のあとにスペースを空けることをおすすめいたします。

Copy link
Owner Author

Choose a reason for hiding this comment

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

全体的にスペースあたりの箇所で指摘をいただいているので、
次回以降はフォーマットにも気をつけて提出するようにします。


for row in range(num_of_rows):
for col in range(num_of_cols):
if grid[row][col] == LAND:
Copy link

Choose a reason for hiding this comment

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

L31は処理上はいらないと言えばいらないですね。(L17があるので)
ただなくすと無駄な関数呼び出しは発生するので、選択が難しいですね

def maxAreaOfIsland(self, grid: List[List[int]]) -> int:
LAND = 1
WATER = 0
VISITED = -1
Copy link

Choose a reason for hiding this comment

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

破壊的というだけでなく、個人的にVISITEDはLANDかWATERとは別の次元の話だと思っており、個人的には別で定義してるStep2の最後の方が好きです

LAND = 1
WATER = 0
DIRECTIONS = [(1, 0), (0, 1), (-1, 0), (0, -1)]
visited = set()
Copy link

Choose a reason for hiding this comment

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

setではなく二次元配列で定義する方法もありますね。
オーバーヘッドは少ないかもですが、メモリは多く使いそうです

Copy link

Choose a reason for hiding this comment

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

同じく使用メモリ量の増える方法ですが、gridのdeepcopyを作ってそこに対してVISITEDのマークをつける、という方法もあるかなと思いました

if (row, col) in visited:
continue
if grid[row][col] != LAND:
continue
Copy link

Choose a reason for hiding this comment

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

if (row, col) in visited:if grid[row][col] != LAND:を二つの条件式に分ける意味があまりないのではと思いました。
自分ならif grid[row][col] != LAND or (row, col) in visited):if not (grid[row][col] == LAND and (row, col) not in visited):のどちらかで書くかなと思いました

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.

6 participants