Create Valid Parentheses#11
Conversation
Refactor variable naming for clarity and improve condition check.
| 今更だが、`if len(visited)==0`より`if not visited`のほうが良さそう | ||
|
|
||
| ```python | ||
| from collections import deque |
There was a problem hiding this comment.
stackの用途だけであれば通常のリストがそのままスタックとして使えます
https://docs.python.org/ja/3.11/tutorial/datastructures.html#using-lists-as-stacks
There was a problem hiding this comment.
queueとして使わなければlistでいいんですね。先頭が変わらなければlistの先頭のポインタの更新をしなくていいからということだと理解しました。（step2でdocs読むようにします）
| "}": "{", | ||
| "]": "[" | ||
| } | ||
| visited = deque([]) |
There was a problem hiding this comment.
グラフなどを探索しているわけではないのでvisitedという名前は少し違和感がありますね…
open_bracketsなどでよさそうです
There was a problem hiding this comment.
visitedにopen_bracket側しか入らないことに指摘されて気づきました。open_bracketsいいですね。
この行の段階でvisited(open_brackets)がsを舐める変数であることを読めるように変数名に要素を追加したいのですが、いい名前が思いつきません。
| if visited: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
こちらはifの条件をそのまま返り値にできそうです
| if visited: | |
| return False | |
| return True | |
| return not visited |
There was a problem hiding this comment.
他の指摘も含め, step4で採用しました。ありがとうございます。
| elif visited and visited[-1] == CLOSE_TO_OPEN_BRACKETS[character]: | ||
| visited.pop() | ||
| else: | ||
| return False |
There was a problem hiding this comment.
visitedからpopしたものをそのまま判定に使うと条件分岐が減らせるかなと思います
| CLOSE_TO_OPEN_BRACKETS = { | ||
| ")": "(", | ||
| "}": "{", | ||
| "]": "[" | ||
| } |
There was a problem hiding this comment.
この書き方はぱっと見で対応が取れているか分かりづらい（閉じ→開きなので順序が不自然）ので
、開き括弧→閉じ括弧にしてスタックからポップした開き括弧をこの対応に入れて一致を見るという方法もあります
| taple_of_brackets = (('(', ')'), ('{', '}'), ('[', ']')) | ||
| visited = deque([]) | ||
|
|
||
| def is_valid(character): |
There was a problem hiding this comment.
名前がis_validなのは少し不自然かなと思いました、外のisValidとも（大文字小文字などの違いはありますが）実質名前が被ってしまっていますので混乱しそうです。
気持ちとしてはprefixを見たときに括弧の対応が破綻していないかという感じだと思うのですが、ちょっとぱっとは良い名前が思いつきませんでした
| visited = deque([]) | ||
|
|
||
| def is_valid(character): | ||
| if character not in left_side_of_brackets: |
There was a problem hiding this comment.
if character in left_side_of_bracketsの場合を早期リターンするとネストがインデントが減らせそうです
| # pythonではlistでも実装できるが、趣旨に沿ってdeque(純粋なstackはなかったはず)で実装する | ||
|
|
||
| left_side_of_brackets = ('(', '{', '[') | ||
| taple_of_brackets = (('(', ')'), ('{', '}'), ('[', ']')) |
| return False | ||
| if character in left_side_of_brackets: | ||
| visited.append(character) | ||
| print(visited) |
| # pythonではlistでも実装できるが、趣旨に沿ってdeque(純粋なstackはなかったはず)で実装する | ||
|
|
||
| left_side_of_brackets = ('(', '{', '[') | ||
| taple_of_brackets = (('(', ')'), ('{', '}'), ('[', ']')) |
There was a problem hiding this comment.
変数名に型名をつけても、読む人にとってあまり有益にならないかなと思います。open_to_closeとかはどうでしょうか。
| left_side_of_brackets = ('(', '{', '[') | ||
| taple_of_brackets = (('(', ')'), ('{', '}'), ('[', ']')) | ||
|
|
||
| visited = deque([]) |
There was a problem hiding this comment.
空の相当キューの初期化であれば
visited = deque()のほうがシンプルだと思います。
| if c in left_side_of_brackets: | ||
| visited.append(c) | ||
| else: | ||
| if len(visited) == 0: |
There was a problem hiding this comment.
Implicit false を利用して、
if not visited:と書くのはいかがでしょうか？
参考までにスタイルガイドへのリンクを貼ります。
https://google.github.io/styleguide/pyguide.html#2144-decision
For sequences (strings, lists, tuples), use the fact that empty sequences are false, so if seq: and if not seq: are preferable to if len(seq): and if not len(seq): respectively.
上記のスタイルガイドは唯一絶対のルールではなく、複数あるスタイルガイドの一つに過ぎないということを念頭に置くことをお勧めします。また、所属するチームにより何が良いとされているかは変わります。自分の中で良い書き方の基準を持ちつつ、チームの平均的な書き方で書くことをお勧めいたします。
| if character in left_side_of_brackets: | ||
| visited.append(character) | ||
| print(visited) | ||
| if len(visited) == 0: |
| visited = deque([]) | ||
|
|
||
| def is_valid(character): | ||
| return character not in left_side_of_brackets and (len(visited) == 0 or (visited.pop(), character) not in taple_of_brackets) |
There was a problem hiding this comment.
一行が長すぎるように感じました。 if 文に分解するか、適宜改行したほうが読みやすくなると思います。
https://google.github.io/styleguide/pyguide.html#32-line-length
| class Solution: | ||
| def isValid(self, s: str) -> bool: | ||
|
|
||
| OPEN_BRACKETS = ('(', '{', '[') |
| class Solution: | ||
| def isValid(self, s: str) -> bool: | ||
|
|
||
| OPEN_BRACKETS = ["(", "{", "["] |
There was a problem hiding this comment.
将来カッコが増えた時に二箇所を修正する必要があり抜けもれが発生しやすいので、OPEN_TO_CLOSE_BRACKETSだけを定義して、開き括弧はkeysで取り出すのが良いと思いました。
There was a problem hiding this comment.
ここまでの練習で変更しやすさ、バグの混入しにくさの視点が抜け落ちてました。
ご指摘ありがとうございます。
5 participants
This problem:
https://leetcode.com/problems/valid-parentheses/description/
Next problem:
https://leetcode.com/problems/generate-parentheses/description/