Conversation
URL: https://leetcode.com/problems/max-area-of-island/description/ You are given an m x n binary matrix grid. An island is a group of 1's (representing land) connected 4-directionally (horizontal or vertical.) You may assume all four edges of the grid are surrounded by water. The area of an island is the number of cells with a value 1 in the island. Return the maximum area of an island in grid. If there is no island, return 0.
| @@ -0,0 +1,220 @@ | |||
| ### Step1 | |||
There was a problem hiding this comment.
特に大きな問題はなさそうですが、前回のレビューの指摘が反映されていないのと、書き方に一貫性があまりないのが気になりました。
#23
There was a problem hiding this comment.
ありがとうございます。コメントの中でDiscord上に何故か出なかったものを見逃してました🙏
| return area | ||
|
|
||
| max_area = 0 | ||
| for r in range(len(grid)): |
There was a problem hiding this comment.
ループの命名がr, cなのがちょっと気になります。num_rows = len(grid)のように変数化されていればまだ推測がつきそうですが、単体でいきなりrが来たら結構頭を悩ます気がします。
| checked = [[False] * len(grid[0]) for _ in range(len(grid))] | ||
|
|
||
| def calculate_area(start_row, start_col): | ||
| if not (0 <= start_row < len(grid) and 0 <= start_col < len(grid[0])): |
There was a problem hiding this comment.
これ外すと、notが前半にしかかからなくておかしくなりませんか
| def calculate_area(start_row, start_col): | ||
| lands_stack = [] | ||
| lands_stack.append((start_row, start_col)) | ||
| inner_index = lambda row, col: 0 <= row < len(grid) and 0 <= col < len(grid[0]) |
| checked[start_row][start_col] = True | ||
| diffs = [(0, 1), (0, -1), (1, 0), (-1, 0)] | ||
| area = 1 | ||
| for rd, cd in diffs: |
|
|
||
| def maxAreaOfIsland(self, grid: List[List[int]]) -> int: | ||
| def inner_index(row, col): | ||
| return 0 <= row < len(grid) and 0 <= col < len(grid[0]) |
There was a problem hiding this comment.
len(grid), len(grid[0])は何度か使われているので、変数化してしまったほうが読みやすく感じるかと思います。
There was a problem hiding this comment.
for i in range(len(grid)) て書いた方がiがindexの中を回してるってことがわかりやすいのと同様に、これもあえて変数にしない方がわかりやすいかなーと思ってしなかったんですが、冗長になるかどうかとの選択がなかなか難しいですね、、、
There was a problem hiding this comment.
gridが２次元配列なのでまあり直感的でないかなというのが個人的な感覚です。まあ好みの範囲だと思います。
for i in range(len(prices)): これとかは比較的直感的な気がするんですけど、for i in range(len(grind[0]))だと脳内変換での変換1Step挟まる感じです。
| WATER = 0 | ||
|
|
||
| def maxAreaOfIsland(self, grid: List[List[int]]) -> int: | ||
| def inner_index(row, col): |
There was a problem hiding this comment.
真偽値を返す関数は疑問形にするのが一般的かなと思います
| def inner_index(row, col): | |
| def is_inner_index(row, col): |
加えて、関数の使い方をみると”gridの外かどうか”を知るためだけに使われているので、命名とif文を調整したほうが良いかもしれません
There was a problem hiding this comment.
命名はなるほど、知りませんでした、ありがとうございます。
「if文を調整」とはどういうことでしょうか。
There was a problem hiding this comment.
@nittoco
if not inner_index(start_row, start_col): -> if is_out_of_grid(start_row, start_col):
みたいにnotを消す形を意味してました
There was a problem hiding this comment.
ありがとうございます！
うーん、そっちは個人的にはあまりしっくりこないかも（普通「でない」時、early returnする、という感覚なので)
| def calculate_area(row, col): | ||
| if not is_inner_index(row, col): | ||
| return 0 | ||
| if not grid[row][col] == self.LAND: |
There was a problem hiding this comment.
これはもう少し分かりやすく書き直せませんか？notと==の優先順位は自明ではないと思います。
class member と instance memberの違いはご存知でしょうか？
There was a problem hiding this comment.
class member と instance memberの違い 違い自体はわかってましたが名前と結びついてなかったです。
https://en.wikipedia.org/wiki/Class_variable
今回はどのインスタンスでも共通のクラス変数ですが、selfは各インスタンスに紐づくので、Solution.LANDとクラス名にしたほうが良いのかもですね
6 participants
URL: https://leetcode.com/problems/max-area-of-island/description/ You are given an m x n binary matrix grid. An island is a group of 1's (representing land) connected 4-directionally (horizontal or vertical.) You may assume all four edges of the grid are surrounded by water.
The area of an island is the number of cells with a value 1 in the island.
Return the maximum area of an island in grid. If there is no island, return 0.