Skip to content

number_of_islands#8

Open
tshimosake wants to merge 1 commit into
masterfrom
tshimosake-patch-6
Open

number_of_islands#8
tshimosake wants to merge 1 commit into
masterfrom
tshimosake-patch-6

Conversation

@tshimosake

Copy link
Copy Markdown
Owner

Comment thread number_of_islands.md
```py
class Solution:
def numIslands(self, grid: List[List[str]]) -> int:
def dfs(i:int, j:int) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

変数名については色々と議論があります。
https://discord.com/channels/1084280443945353267/1230079550923341835/1230201155619913728

Comment thread number_of_islands.md
# 範囲外または海なら返す
if not 0 <= i < len(grid) \
or not 0 <= j < len(grid[0]) \
or grid[i][j] == '0':

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

'0'や'1'だと味気ない(reader-friendlyでない)ので、'WATER'や'LAND'のような変数名やEnumを使う選択肢も一考かと思います。

Comment thread number_of_islands.md
return num_of_islands
```

2回目と3回目。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

他にもBFSやUnionFindで書いてみると勉強になるかもしれません。

Comment thread number_of_islands.md

num_of_islands = 0
for i, row in enumerate(grid):
for j, num in enumerate(grid[i]):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

rowに対応するものだとcolが自然でしょうか。
あとrownumって使っていませんね。

Comment thread number_of_islands.md
def numIslands(self, grid: List[List[str]]) -> int:
def dfs(i:int, j:int) -> None:
# 範囲外または海なら返す
if not 0 <= i < len(grid) \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

width = len(grid)のようにしてwidthを使い回した方が良いかもしれません。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

個人的には、not A or not Bよりnot(A and B)の方が分かりやすいです。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

自分はtorusさんと逆で not A or not B の方が読みやすく感じました。

Comment thread number_of_islands.md
or grid[i][j] == '0':
return

grid[i][j] = '0'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

gridを直接書き換えても良いのかどうかも少し考えたいポイントですよね。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

これは本当にそうですね。あとで書いてみます。

Comment thread number_of_islands.md
```py
class Solution:
def numIslands(self, grid: List[List[str]]) -> int:
def dfs(i:int, j:int) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

: のあとにスペースを空けることをお勧めいたします。
https://peps.python.org/pep-0008/#other-recommendations

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

リンク先だとむしろ行末にスペースを入れるなと書いてありませんか?

Avoid trailing whitespace anywhere.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i:int のほうです。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

なるほどです。ありがとうございます!

Comment thread number_of_islands.md
def numIslands(self, grid: List[List[str]]) -> int:
def dfs(i:int, j:int) -> None:
# 範囲外または海なら返す
if not 0 <= i < len(grid) \

Copy link
Copy Markdown

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#s3.6-whitespace

Do not use a backslash for explicit line continuation.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

知りませんでした。ありがとうございます!

Comment thread number_of_islands.md
or grid[i][j] == '0':
return
grid[i][j] = '0'
dfs(i+1, j)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 回目のように + や - の左右にスペースを空けたほうが自然に感じました。

Comment thread number_of_islands.md
Comment on lines +8 to +10
if not 0 <= i < len(grid) \
or not 0 <= j < len(grid[0]) \
or grid[i][j] == '0':

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

改行がかえって見づらいように感じました。改行する場合にも if と同じインデントの部分に or がくるのは違和感があります。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

こんな感じでしょうか

if (a
    or b
    or c
):
    pass

Comment thread number_of_islands.md
Comment on lines +15 to +18
dfs(i + 1, j)
dfs(i, j + 1)
dfs(i - 1, j)
dfs(i, j - 1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

for ループを使うやり方もありますね。

Comment thread number_of_islands.md
Comment on lines +46 to +49
for i, _ in enumerate(grid):
for j, char in enumerate(grid[i]):
if char == '1':
res += 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

enumerate を使うことで可読性が落ちていそうです。シンプルに二重で for ループを回し、grid[i][j] にアクセスするほうがわかりやすいと思いました。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

個人的に range(len(grid)) と2回関数を適用すると見づらいと思っており、enumerateを使いました。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

見づらいというのは僕も同意です。なので m = len(grid), n = len(grid[0]) とはじめに宣言しておくと、for i in range(m) のようになるのでわかりやすそうに思いました。(問題文中で m x n のサイズと明記されているので grid の範囲内かどうかの判定でも m, n を使うとわかりやすくなりそうです)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ああ、たしかにそれはいいですね!

Comment thread number_of_islands.md
def numIslands(self, grid: List[List[str]]) -> int:
def dfs(i:int, j:int) -> None:
# 範囲外または海なら返す
if not 0 <= i < len(grid) \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

自分はtorusさんと逆で not A or not B の方が読みやすく感じました。

Comment thread number_of_islands.md
if not 0 <= i < len(grid) \
or not 0 <= j < len(grid[0]) \
or grid[i][j] == '0':
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

この条件を再帰呼び出しの前に移動することで、再帰呼び出しの回数を最小化することもできます。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

逆に関数側で処理したほうが呼び出すときに柔軟に扱えて便利だとも思っています。関数の呼び出しは(ほかの処理に比べて)パフォーマンスに大きく影響するのでしょうか?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

関数呼び出しは足し算や掛け算と比べると遅いですが、大きく影響するものではないです。
各オペレーションに必要なCPUクロック数の目安です -> http://ithare.com/infographics-operation-costs-in-cpu-clock-cycles/

自分も両方書くので好みの問題ですが、例えば再帰を非再帰に書き換えるときなどに有用になると思います。

Comment thread number_of_islands.md
dfs(i, j+1)
dfs(i-1, j)
dfs(i, j-1)
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ここのreturnは省略できると思います。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

好みかもしれませんが、Pythonはインデントで関数の範囲?が解釈されるので、returnを明示したほうがわかりやすいかな、と思っています。

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.

5 participants