-
Notifications
You must be signed in to change notification settings - Fork 0
20 https://leetcode.com/problems/valid-parentheses/ #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
class Solution: | ||
def isValid(self, s: str) -> bool: | ||
parentheses_stack = [] | ||
close_to_open = {")":"(", "]":"[", "}":"{"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:
のあとにスペースを 1 つ入れましょう。
https://google.github.io/styleguide/pyguide.html#36-whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
見返してかなり見づらいとわかりました。書いた直後の段階で気づけるようにしたいと思います。リファレンスをお示しいただきありがとうございます。
def isValid(self, s: str) -> bool: | ||
brackets_stack = deque() | ||
close_to_open = {")":"(", "]":"[", "}":"{"} | ||
open_bracket_chars = set(close_to_open.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step 2 で書いた open_brackets
で十分だと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
文字の set であることが伝わりやすくなるかと思ったのですが、長くすればよいというものではないですね。bracketsの時点で (複数形なので) 何らかのコンテナ型だと伝わりそうですし。
なかなか勘所が難しいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これくらいの長さだったら in 文字列 のほうが速そうですね。
cProfile.run('for i in range(10**8): "[" in s')
s = "({[" -> 5.395
s = set('({[') -> 5.510
close_to_open = {")":"(", "]":"[", "}":"{"} | ||
open_brackets = set(close_to_open.values()) | ||
close_brackets = set(close_to_open.keys()) | ||
brackets = set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brackets = open_brackets | close_brackets
と書けると思います。
https://docs.python.org/ja/3/library/stdtypes.html#set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
union演算が頭にありませんでした!集合演算を用いて簡潔にsetを操作できるようにしたいと思います。リファレンスをお示しいただきありがとうございます。
pass | ||
elif char in "([{": | ||
parentheses_stack.append(char) | ||
else:# iff char in ")]}": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif not parentheses_stack or parentheses_stack.pop() != close_to_open[char]:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if-elif-elseの対応がついている方がcharの種類に応じて場合分けをしている感じが伝わるかな、と思ったのですがこれぐらいだと、else内部のifの条件分をそのまま書いてしまっても変わらないということでしょうか。
どれぐらいまでの書き換えが自然に感じるのか、という感覚が自分はまだ甘いなと思います。
else:# iff char in ")]}": | ||
if not parentheses_stack or parentheses_stack.pop() != close_to_open[char]: | ||
return False | ||
if parentheses_stack: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return not parentheses_stack
(下のノートにありましたね。)
if char in open_bracket_chars: | ||
brackets_stack.append(char) | ||
continue | ||
if char in close_bracket_chars: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not brackets_stack or brackets_stack.pop != close_to_open[char]:
return False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここは考え方として、分けてもつけてもある程度は趣味の範囲と思いますが、くっつけるのも自然だと思います。
if index < len(array) and is_valid(array[index]):
とか
if index >= len(array) or not is_valid(array[index]):
これ、一動作として自然ですよね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
おっしゃるとおり「データにアクセスできて、ある条件を満たしている (ならOK)」か「データにアクセスできないか、アクセスできてもだめ」のように連結したほうが意味的に自然だと感じました。
この問題では
if index < len(array) and is_valid(array[index]):
continue
return False
とするのが、他の条件文とif-continueのパターンが同じになってわかりやすいかなと感じました。
・return Falseを実行する条件文を2つに分解した。 | ||
まとめて書くと80列をオーバーしたため。 | ||
PEP8によれば、次のように書いてもいい | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://formatter.org/python-formatter
フォーマッターに入れて確認してみてもいいですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
フォーマッターに入れたところ
if not parentheses_stack or parentheses_stack.pop() != close_to_open[char]:
return False
は特に修正されませんでした。これぐらいの長さであれば意味的なつながりを優先したほうが良さそうですね。
if char in "([{": | ||
parentheses_stack.append(char) | ||
continue | ||
if char in ")]}": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このifは不要です。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この ifに引っかからない要素は意図していないので、分岐をすり抜けて無視されたほうがよいかなと思い if で書いていました。無視されたほうがいいか、どこかに引っかかったほうがいいかはケースバイケースだと思うのですが、作るときの都合と読むときの都合を別に考えて見直そうと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このifに入らないケースはありますか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この条件文の構成ではないと思います。(if char not in "()[]{}" , if char in "([{", if char in ")]}" が排反で、charは必ずどれかのパスに入るので)
以下のように、if char not in "()[]{}" を明示しない構造も選択肢にあり、あとからこの if 文を追加したのもあり、一番下の if 文がそのままになっていました。
for char in s:
if char in "([{":
(処理)
continue
if char in ")]}:
(処理)
すり抜け云々はデバッグのやりやすさを意図していました。間違えていた場合、今回はすりぬけがないものをデバッグするよりも、すり抜けがあるものをデバッグするほうが簡単かなと思って条件文を3つとも残していました。
(forループの末尾に、print("unexpected char", char)
的なものを追加しやすそうかなと)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
読み手としては、よりシンプルな書き方があるのに違う選択をしているのは不自然に感じました。
- 後ろからしか出し入れしないのに、deque
- elifではなく、else: if
- 必ずtrueになるif文
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- elifではなく、else: if
- 必ずtrueになるif文
のいずれも、自分の頭の中の意味的な整理に由来する冗長性のように思えたので、できるだけ削れないかという視点で見直すようにします。
データ構造も最小限のものを選ぶようにしてみます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「必ずtrueになるif文」は、書き手が何を伝えたかったのが謎になるので、せめて、コメントか assert にしておくといいでしょう。
基本的には、エンジニア同士は、「書いた人はそれなりの選択肢が見えていて、わざわざそれを選んだことをシグナルとして読み手に送っている」という前提で読んでいます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「必ずtrueになるif文」が与える印象を想定できていませんでした。assert を使ったほうがすり抜けを明示的に扱えるので適切ですね。
elif に関して、 numpy を読んでみたところ、else: が続かない elif も普通に使われていたので、elif を else で閉じたくなるのは自分の癖だと認識できました。色々読んでみて、普通はどうするか、の感覚を掴んでいこうと思います。
from collections import deque | ||
class Solution: | ||
def isValid(self, s: str) -> bool: | ||
brackets_stack = deque() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dequeを使うメリットはありますか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今回の問題の範囲では append, pop しか使っておらず、 時間計算量としては末尾への追加・削除だけを行う範囲では list を使っても違いはないと思います。
文字列が動的に変更される場合は、先頭側への挿入・削除処理もO(1)で実行したいので list ではなく dequeを使うのが適していると思います。
discord内でこの問題に関して deque が参照されており、スタック・キューとして deque が用意されていたので使ってみました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、メリットはありますか? という質問なのは、
- そもそもプログラムを書くという行動は、随所で選択肢から選んでいく行為である
https://hayapenguin.com/notes/Posts/2024/04/24/how-to-practice-coding-effectively - 普通は標準的なものを選ぶ
- わざわざ変わったものを選んだということは、何かが見えているに違いない
- なにか読み手に伝えようとしている内容があるはずだ
と感じているからです。
逆に、list を選ぶ理由としては、
- より簡単な構造である
- スタックとしてしか使わないことがわりと明確
- この問題は、プッシュダウンオートマトンをイメージするのでスタック
あたりがあります。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
機能的に十分以上のデータ構造をつかったら、それをつかわないとできないなにかがあるのでは、という余計な推測をさせてしまうということですね。
最後まで読んで、結局何をしたかったのかよくわからない、というのは読み手への負荷が高いので、余分な意味を持たない表現、構造を意識してみます。
close_to_open = {")":"(", "]":"[", "}":"{"} | ||
for char in s: | ||
if char not in "()[]{}": | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私は、この pass は嫌で、目を5行下に動かして continue と同じだと分かるわけです。目の移動量を増やしています。
https://discord.com/channels/1084280443945353267/1233603535862628432/1235918215989956658
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passは読み手に想像させるものが continueと明確に違うという感覚がありませんでした。この pass は分岐のあとに処理が続くことを予告することになっていますね。構造を書くとき/見るときの感覚として持っておかなければならないと思いました。
リファレンスをお示しいただきありがとうございます。
https://leetcode.com/problems/valid-parentheses/