Skip to content

22. Generate Parentheses#6

Open
atmaxstar wants to merge 1 commit into
mainfrom
22
Open

22. Generate Parentheses#6
atmaxstar wants to merge 1 commit into
mainfrom
22

Conversation

@atmaxstar
Copy link
Copy Markdown
Owner

if m == 0:
return parenthesis
next_parenthesis = set()
for parenthe in parenthesis:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ブラ/ケットに近い命名だとは思いますが、parentheは普通の単語ではないので、ちょっと違和感を持つ人が多いかなと思います。特に、parentheだと"("みたいなのを想像するかなと思います。
parenthesis自体が単数形なので、next_parenthesisともども複数形のparenthesesを使い、parenthe -> parenthesis がよいかなと思います。
英文でコードを書くときは、多くのエディタではタイポを指摘されると思っていて、parentheだとタイポ指摘が存在する状態になる、というのもあります。
以下、指摘の例(これはブラウザですが)↓

Image

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.

parenthesisが単数形であることは後々知りました。最初はparenthesesを単数形にするとなんだろうとなって結局parentheとぶつ切りにする感じにしてしまいました...
実務だときちんと調べて正確な単数形にすると思うのですが面接中などは思いつきの単語で回すか面接官に聞くしかなさそうですね...

# when facing ), put () before )
# when facing "", put ()
def generateParenthesis(self, n: int) -> List[str]:
answer = []
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.

この変数使ってないですね。削除しておくべきですがもし使うとしたらそのままparenthesisと名付けるのが自然ですかね

def backtrack(parenthesis, m):
if m == 0:
return parenthesis
next_parenthesis = set()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

この問題では順番は求められていませんが、一般にsetを使った結果をそのままlist()して返却すると、順番はぐちゃぐちゃになります。

next_parenthesis.add(parenthe[:i] + "()" + parenthe[i:])
return backtrack(next_parenthesis, m - 1)

def find_index_of_corresponding_symbol(part):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

この関数名で突然symbolが出てきているので、parenthesisで良いかなと思います。ただ、関数名が長いので、find_index_of_closerぐらいが良いかなと思いました。

が、よくよく考えると、partを受け取って最初の(が閉じる位置を返却する、なので、(未定義ですが)実態としてはfind_first_edge(_index)とかfind_first_break(_index)とかですかね。

closing += 1
if opening == closing:
return i
return -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.

この関数単体で見たときに-1はよくあるfindの動作だと思いますが、このプログラムが正しく動作している限りにおいてはここには来ないので、そのコメントがあると親切かなと思いました。

```

## step3:
どうせなのでbacktrack, bfsで解いてみる。
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

step3は、最終的に自分が妥当だと思うコードをすぐに書けるようにする定着の目的もあります。意図的に自分が最も妥当だと思うコードを選んで練習するのがよいと思います。
また、他の人のPR・コードを読んで、それについての感想などを書くと、それも自分が書くコードの品質を良くすると思います。

class Solution:
def generateParenthesis(self, n: int) -> List[str]:
answer = []
parentheses_and_open_and_close = []
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

これは「(parentheses: str, open: int, close: int)のtupleと同じ中身のlist」のlistだと思いますが、名前がかなりわかりにくいです。
pythonであっても型を明示するようにしたほうが、練習としては効果的ではないかと思います。
(parentheses: str, open: int, close: int)の部分はまずtupleでよく(listにするとlist[str | int]になってしまうし長さも固定されない)、

parentheses_and_open_and_close: list[tuple[str, int, int]] = []

とかでよいと思います。
関数名は、冗長ですがparentheses_with_counts_stackとかですかね。
単にstackとかstatesみたいなものもAIには提案されました。

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.

AIの提案通りにstackやstatesにすると個人的には何が入ってるか迷ってしまいそうなので
parentheses_and_open_and_close: list[tuple[str, int, int]] = []
これが最も読みやすいと感じますね。parentheses_with_counts_stackもコメントなどでparentheses, open, closeがどう返ってくるかを添えておけば読みやすいと思います。

parentheses, open_num, close_num = parentheses_and_open_and_close.pop()
if open_num == close_num == 0:
answer.append(parentheses)
continue
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

このcontinueは後ろのifの評価を避ける程度の意味しかないので、なくてよいかなと思いました。好みの問題かもしれません。

continue
if open_num > 0:
parentheses_and_open_and_close.append([parentheses + "(", open_num - 1, close_num])
if close_num > 0 and close_num > open_num:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

close_num > 0は、open_numが負にならないので不要ですかね。
そもそも、この処理の中でopen_num <= close_numは保証されているので、最初のopen_num == close_num == 0もclose_num == 0で十分と思いました。

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.

close_num > 0は、open_numが負にならないので不要ですかね。

これはそうですね。

この処理の中でopen_num <= close_numは保証されているので、最初のopen_num == close_num == 0もclose_num == 0で十分と思いました。

これも正しいのですが、open_num, close_numを残りの"(", ")"を置ける数と定義している以上最後はopen_num == close_num == 0と置いた方が読み手に終了条件が伝わりやすくていいかなと個人的に思いました。

parentheses_and_open_and_close = []
parentheses_and_open_and_close.append(["", n, n])
while parentheses_and_open_and_close:
parentheses, open_num, close_num = parentheses_and_open_and_close.pop()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

open_num, close_numはremaining_open, remaining_closeみたいな、残りのopen数、残りのclose数、みたいなことがわかる変数名のほうが適切かなと思いました。
もしopen_num, close_numを維持するなら、処理を["", 0, 0]で始めて、+-を逆転させて、close_num == n、open_num < n、close_num < open_num、でそれぞれ判定するのがよいかなと思います。

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.

2 participants